Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: flag to return return a failure exit code #474

Open
1 of 2 tasks
nwsparks opened this issue Apr 22, 2024 · 8 comments
Open
1 of 2 tasks

Feature request: flag to return return a failure exit code #474

nwsparks opened this issue Apr 22, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@nwsparks
Copy link

The feature request

flag to return return a failure exit code if any repo(s) do not succeed.

apologies if I missed that this is currently available somewhere, did not see it.

My use case

Would like to use multi-gitter with some cron jobs for package updating and would like a clean way of notifying when it fails on a repo.

Implementation

  • I would like to contribute this feature if it's a suitable addition to multi-gitter
  • I have no intention of adding this feature myself.
@nwsparks nwsparks added the enhancement New feature or request label Apr 22, 2024
@nwsparks nwsparks changed the title Feature request: Feature request: flag to return return a failure exit code Apr 22, 2024
@jcmcken
Copy link

jcmcken commented Jun 15, 2024

This also makes it more difficult to run in a CI context, where a failure should indicate to the CI system that the operation was not successful. That's usually signaled with the exit code

@lindell
Copy link
Owner

lindell commented Jun 17, 2024

This is a good idea. However, the option would also have to define what the user defines as an error.

Some things that could be considered an error:

  • Error code in a script
  • No change by script
  • Fail to pull/push etc.
  • Skip because of already existing branch
  • Probably more that I'm missing

Not all of these might be considered an error by all users.

@nwsparks
Copy link
Author

I would keep it simple and just deal with execution errors of any kind. I think that covers the vast majority of use cases. You'll get lost in the weeds otherwise.

I don't think skips, no changes should be considered for this. Things like no change could be dealt with in the script by detecting it and throwing an error if people really want that behavior.

If you felt it was really necessary to get so granular you could start with a flag for execution errors and build on with more flags later for the other scenarios?

@lindell
Copy link
Owner

lindell commented Jun 17, 2024

@nwsparks It might be the most common use case, but just look at the reference from another comment @jcmcken made 3 days ago. That seems to have been about skips not causing errors.

@nwsparks
Copy link
Author

Hmm, I reviewed both comments and didn't see a specific mention about skipping. I think your point is a good one, it seems like a separate issue though that would be handled by additional flags or redesign of existing flags such as --conflict-strategy error

@jcmcken
Copy link

jcmcken commented Jun 17, 2024

From my point of view, it makes sense to error in cases where doing the same operation by hand in a shell would have a non-zero exit code (even though this tool may not be using any CLIs directly). After all, kind of the point of multi-gitter (as I see it at least) is to save you from having to do these operations by hand.

So to me, that includes everything in your list

  • failure to push/pull -- propagate git's error handling, which is to fail
  • branch already exists -- here, git should just error because the remote has commits that local does not.
  • the script itself errors -- makes sense to propagate this error
  • empty changes (i.e. you run the script, and it produces no changes to any files) -- if you try to git commit without any staged changes, it errors (also makes sense not to build a PR for an empty changeset)

@nwsparks
Copy link
Author

Thanks for the discussion and sorry if I'm misunderstanding, it sounds like you are saying all of those would be part of this new feature? I agree on script failure and git push/pull failure.

branch already exists

Aren't you already dealing with this via --conflict-strategy? If someone wants exit codes and also --conflict-strategy replace how would that work? I think it becomes confusing and unclear if you include it here. It would make more sense to expand --conflict-strategy if additional behavior for existing branches is desired. I don't think this should be added as part of this feature request.

empty changes

The use case for adding exit codes would primarily be for automated execution, in our case we're using the tool to do nodejs package updates weekly across 50+ repositories. Executions that result in no changes are not an error, it just means that no updates were found. If you make no changes result in an error then we are no better off than we were before and would get no benefit out of this new feature. I suspect that this would be the case for many who would use this feature.

I think again if this behavior is desired (fail on empty changes) it needs to be a separate flag and unrelated to the feature request here.

@jcmcken
Copy link

jcmcken commented Jun 18, 2024

@nwsparks

Some of it is up for interpretation I think. I'm just trying to give a backing theory for how to design the exit codes

For replace conflict strategy, I think it's straightforward. If you force push a branch with git CLI (git push -f) where you have conflicts between local and remote, it should exit with a 0 exit code, with remote getting deleted and replaced by local.

Normally, if you use plain git push (no -f), it should error since the remote and local have diverged. But since the conflict strategy skip already implies a specific behavior (continue without erroring), that should not change, so the exit code would be 0. (I'm not advocating for any backwards incompatible change). In this case, the CLI analogue (how you could accomplish this behavior just using the git CLI and shell commands) is more like you're running git ls-remote to check if the remote branch exists already, and if it does, continuing without action. Then maybe it makes sense to have another conflict strategy called error as you stated

The issue I want to address with branches is when there are many developers altering the same GitOps repos. If you're modifying 50 or 100 repos, and 1 is skipped because someone else has already created the named branch, it's likely that you will miss this happened unless you're carefully observing the output. In cases like this, an exit code is a single, unifying signal that tells the user they need to carefully read the logs and take some action. Otherwise the user needs to know the specific content of log messages, use things like grep, and so on -- a more brittle method of error detection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants