Skip to content

fix: support no changes#67

Merged
abannachGrafana merged 5 commits into
grafana:mainfrom
gmeligio:no_changes
Apr 10, 2025
Merged

fix: support no changes#67
abannachGrafana merged 5 commits into
grafana:mainfrom
gmeligio:no_changes

Conversation

@gmeligio
Copy link
Copy Markdown
Contributor

@gmeligio gmeligio commented Apr 5, 2025

Currently, if a working directory has no changes, the step will finish with a failure. This is unexpected since nothing wrong happened, and it's just that there is no need to make a commit. That no-changes can be handled by not running the making the Github API request. In that case, there would be no commit-reponse.

Note: I haven't run the Github workflows for testing yet. It looks like they should be approved on this PR before running.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 5, 2025

CLA assistant check
All committers have signed the CLA.

@gmeligio gmeligio marked this pull request as ready for review April 5, 2025 12:37
Comment thread action.yml Outdated
@abannachGrafana
Copy link
Copy Markdown
Contributor

abannachGrafana commented Apr 9, 2025

I'll need to investigate why it's failing to run the tests. This is happening on #66 as well. So I don't believe it's localized to your branch.

Copy link
Copy Markdown
Contributor

@abannachGrafana abannachGrafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add optional input of succeed-if-no-changes with a default of false to control the exit code. This will preserve existing functionality while supporting an option where no changes committed is ok in a workflow where that's ok.

@abannachGrafana
Copy link
Copy Markdown
Contributor

@gmeligio I've merged #68 to hopefully resolve the test run issue. Please update your branch and we'll see if the tests run successfully.

@gmeligio
Copy link
Copy Markdown
Contributor Author

gmeligio commented Apr 9, 2025

@gmeligio I've merged #68 to hopefully resolve the test run issue. Please update your branch and we'll see if the tests run successfully.

Thank you @abannachGrafana. Rebased but the workflows need to be approved.

@gmeligio
Copy link
Copy Markdown
Contributor Author

gmeligio commented Apr 9, 2025

Please add optional input of succeed-if-no-changes with a default of false to control the exit code. This will preserve existing functionality while supporting an option where no changes committed is ok in a workflow where that's ok.

Thank you for reviewing @abannachGrafana . My idea comes from using the git CLI and the stefanzweifel/git-auto-commit-action action which don't fail if there are no changes.

grafana/github-api-commit-action it's better than what I knew before since it pushes verified commits and it's easier to configure.

Aside from my proposal being a breaking change, I'd think that detecting if there were changes in the working dir can be done with a separate step like git status or git diff or even with the tj-actions/changed-files action. So we could even not create any variable at all.

I understand your suggestion of compatibility and we can still go with success-if-no-changes but I think that could be the default expected as a git user, and if any change detection is supported in the action, it might be the opposite fail-if-no-changes.

Before proceeding with the change, what do you think?

@abannachGrafana
Copy link
Copy Markdown
Contributor

Re: the workflow runs

Thanks! The other PR dev also did so and we still have a failure due to repo permissions. I'm in progress on a fix for that, but am waiting on a request to go through.

I think that could be the default expected as a git user

Correct as a git user in the terminal you would realize the error and could correct it if there were no changes staged. Since github actions are an automated workflow with no way to intervene I still believe the action should fail by default if the implementor forgot or failed to change files when its expected that there should be a change.

Consider something like an "Update changelog" workflow that is supposed to add PR info to the changelog. If the changelog wasn't changed or the workflow creator forgot to stage the file in a step. They may think everything is fine if the commit step was successful.

a separate step like git status or git diff or even with the tj-actions/changed-files action

Yes! I'd pose that this should be done prior and to skip calling the commit step all together with a conditional on the step. I'd also think that if a workflow may or may not make changes, it'd be better structured in that way as well so that it's clear it's possible that nothing is committed.

I'd like to stick with the success-if-no-changes flag. This preserves backwards compatibility and the intention of the action.

@gmeligio
Copy link
Copy Markdown
Contributor Author

Got it. Thank you for the explanation @abannachGrafana . I added success-if-no-changes. It's ready for another review.

The test.yml workflow is running now but it's not running the two tests I added.

@abannachGrafana
Copy link
Copy Markdown
Contributor

Yeah, I'm fighting with the tests because the generated GITHUB_TOKEN doesn't have write permission with event type of pull_request. I've been trying a bunch of stuff. So far no luck. Will need to give up for today, but I did merge a change so that it won't trigger tests on forked prs. It's just unfortunate.

Copy link
Copy Markdown
Contributor

@abannachGrafana abannachGrafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great. The tests make sense too. Just a few suggestions

Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
Comment thread .github/workflows/test.yml Outdated
@abannachGrafana
Copy link
Copy Markdown
Contributor

you'll wanna pull from main again too

@gmeligio
Copy link
Copy Markdown
Contributor Author

Thanks. Applied the suggestions.

@abannachGrafana abannachGrafana merged commit 665f5a0 into grafana:main Apr 10, 2025
3 checks passed
@abannachGrafana
Copy link
Copy Markdown
Contributor

thanks @gmeligio !

@gmeligio gmeligio deleted the no_changes branch April 10, 2025 19:21
@gmeligio
Copy link
Copy Markdown
Contributor Author

Hey @abannachGrafana, could you please release this?

@abannachGrafana
Copy link
Copy Markdown
Contributor

@gmeligio New release 0.4.0 has been created. If you find yourself in a bind next time and need to reference changes to an action that were merged to main, but hasn't had a proper release yet you can either use <action>@main (or whatever the default branch is) or <action>@<commit-hash> and that should allow you to use the latest code.

@gmeligio
Copy link
Copy Markdown
Contributor Author

Thank you! I got it today with renovate. I'll take what you mentioned into account for next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants