update github.com/urfave/cli to v2 #1233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github.com/urfave/cli has started using go modules, and has released v2 with some breaking changes to the API. Nothing major, mostly just cosmetic changes. I don't think there's any change in behavior that affects us. I followed the migration guide at [1], then fixed the remaining compiler errors. [1] https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md
pda
approved these changes
Jun 29, 2020
yob
added a commit
that referenced
this pull request
Jul 20, 2020
This method is used to ensure Environment Variables that configure the agent process aren't automatically passed down to the child process that runs each job. We want to be very explicit about which variables end up in the job process. It's implemented using reflection on the flag structs from the ufave/cli package we use for CLI structure. It checks the EnvVar field on the flag struct, which works just fine with ufave/cli/v1. However, that field doesn't exist any more in ufave/cli/v2 and this method has started silently failing since we upgraded to v2 in #1233. I haven't added tests because no files in this package have tests and I'm not 100% sure where to start adding them. However, I'm treating this like an assertion. If the reflection doesn't work the way we expect, we hard exit with an error, like other should-never-happen errors in agent_start.go. I'll fix the method to work with ufave/cli/v2 in the following commit.
|
I think this actually changed a bunch of how the CLI worked, with this heading in particular being dangerous for us: https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md#flags-before-args This means that, for instance, this command: buildkite-agent meta-data get my-key --default fallbackdoesn’t do what the user expects (the “default” flag in this is just completely ignored), and as of this patch, it is required to be written like this: buildkite-agent meta-data get --default fallback my-keywhich is a major, breaking change. |
|
yer, what a nasty change 😢 There's some discussion here too #1285 |
|
There’s some discussion of making it less stringent in urfave/cli#1113, but it doesn’t look like it’s moving too fast. |
yob
added a commit
that referenced
this pull request
Sep 9, 2020
In #1233 we upgraded github.com/urfave/cli from v1 to v2. There was no functional need for the upgrade, it was just about bumping our dependencies and continuing to slowly adopt go modules. However, I overlooked that v2 changed some rules for ordering of flags and arguments. From the [upgrade guide](https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md): > In v2 flags must come before args. This is more POSIX-compliant. You may need to update scripts, user documentation, etc. > > This will work: > > cli hello --shout rick > > This will not: > > cli hello rick --shout We're not keen on breaking our users scripts, so let's rollback to v1. There's an [open issue upstream](urfave/cli#1113) about the new ordering rules. We'll keep an eye on that, and maybe re-try the upgrade if it becomes possible to do so in a non-breaking way This is mostly a revert of PR #1233, but with a few extra changes to things that happened after that PR (like adding the `artifact search` subcommand).
yob
added a commit
that referenced
this pull request
Sep 9, 2020
In #1233 we upgraded github.com/urfave/cli from v1 to v2. There was no functional need for the upgrade, it was just about bumping our dependencies and continuing to slowly adopt go modules. However, I overlooked that v2 changed some rules for ordering of flags and arguments. From the [upgrade guide](https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md): > In v2 flags must come before args. This is more POSIX-compliant. You may need to update scripts, user documentation, etc. > > This will work: > > cli hello --shout rick > > This will not: > > cli hello rick --shout We're not keen on breaking our users scripts, so let's rollback to v1. There's an [open issue upstream](urfave/cli#1113) about the new ordering rules. We'll keep an eye on that, and maybe re-try the upgrade if it becomes possible to do so in a non-breaking way This is mostly a revert of PR #1233 and #1250, but with a few extra changes to things that happened after that PR (like adding the `artifact search` subcommand).
yob
added a commit
that referenced
this pull request
Sep 9, 2020
In #1233 we upgraded github.com/urfave/cli from v1 to v2. There was no functional need for the upgrade, it was just about bumping our dependencies and continuing to slowly adopt go modules. However, I overlooked that v2 changed some rules for ordering of flags and arguments. From the [upgrade guide](https://github.com/urfave/cli/blob/master/docs/migrate-v1-to-v2.md): > In v2 flags must come before args. This is more POSIX-compliant. You may need to update scripts, user documentation, etc. > > This will work: > > cli hello --shout rick > > This will not: > > cli hello rick --shout We're not keen on breaking our users scripts, so let's rollback to v1. There's an [open issue upstream](urfave/cli#1113) about the new ordering rules. We'll keep an eye on that, and maybe re-try the upgrade if it becomes possible to do so in a non-breaking way This is mostly a revert of PR #1233 and #1250, but with a few extra changes to things that happened after that PR (like adding the `artifact search` subcommand).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
github.com/urfave/clihas started using go modules, and has released v2 with some breaking changes to the API. Nothing major, mostly just cosmetic changes.I don't think there's any change in behavior that affects us.
I followed the migration guide, then fixed the remaining compiler errors.
I recommend reviewing the diff with whitespace changes hidden.