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

refactor: Make CLI argument names consistent #2084

Merged

Conversation

gabyx
Copy link
Contributor

@gabyx gabyx commented May 10, 2022

  • Rename --snapshotMode and --customPlatform and --tarPath

Description

Small refactor to make CLI arguments consistent.

Reviewer Notes

  • The code flow looks good.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Examples of user facing changes:
- Flag `--snapshotMode` is now called `--snapshot-mode`.
- Flag `--customPlatform` is now called `--custom-platform`.
- Flag `--tarPath` is now called `--custom-path`.

@gabyx gabyx changed the title fix: Make CLI arugment names consistent fix: Make CLI argument names consistent May 10, 2022
@gabyx gabyx changed the title fix: Make CLI argument names consistent refactor: Make CLI argument names consistent May 10, 2022
Copy link
Collaborator

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Would it be possible to support both for one release, and log a warning if the old version is set? (And fail if they're both set to different values?)

I think you can do this with:

StringVarP(&opts.SnapshotModeDeprecated, "snapshotMode", "...")
StringVarP(&opts.SnapshotMode, "snapshot-mode", "...")

and validation logic in opts to warn if SnapshotModeDeprecated is set, etc.

This might be a better experience for folks using these flags today. With this change, Kaniko will simply fail and they'll have to read the help output and notice the change in order to proceed.

@gabyx
Copy link
Contributor Author

gabyx commented May 10, 2022

Ah did not know about this, in cobra.

@gabyx
Copy link
Contributor Author

gabyx commented May 17, 2022

@imjasonh: I guess now the PR should be good to go.

@gabyx gabyx force-pushed the feature/correct-cli-argument-naming branch from a3edc76 to 20ab9fb Compare May 17, 2022 17:09
@gabyx gabyx force-pushed the feature/correct-cli-argument-naming branch 2 times, most recently from 11d176c to 00d70af Compare May 31, 2022 20:06
@gabyx gabyx force-pushed the feature/correct-cli-argument-naming branch 2 times, most recently from 661441f to 7276632 Compare July 1, 2022 15:37
@gabyx
Copy link
Contributor Author

gabyx commented Jul 1, 2022

@imjasonh Lets see if the tests run through. They should... Didnt change anything...

@glennakamura
Copy link

glennakamura commented Aug 13, 2022

Is there a reason the --tarPath option wasn't changed to --tar-path?

@imjasonh
Copy link
Collaborator

Is there a reason the --tarPath option wasn't changed to --tar-path?

Probably not.

It'd be worth aligning this as well, if anyone's interested in doing it.

@gabyx
Copy link
Contributor Author

gabyx commented Aug 13, 2022 via email

@gabyx
Copy link
Contributor Author

gabyx commented Aug 13, 2022

@imjasonh: Its fixed now.

Also: I formatted the Readme markdown with prettier. Which is now 80 lines and consistent.
Also: I made the argument section a bit nicer.

@gabyx gabyx force-pushed the feature/correct-cli-argument-naming branch from 5826744 to 5cc8564 Compare August 13, 2022 17:30
@gabyx
Copy link
Contributor Author

gabyx commented Aug 19, 2022

@imjasonh: I have no idea why the executor crashes saying --customPlatform is deprecated?? We are not using this argument...? Do you see it?

@gabyx
Copy link
Contributor Author

gabyx commented Aug 19, 2022

@imjasonh: This can be merged now, thanks :)

I formatted all markdowns, to better have diffs when people add documentation.

@gabyx
Copy link
Contributor Author

gabyx commented Aug 20, 2022

@imjasonh Jeah totally right, we just map the values.

cmd/executor/cmd/root.go Outdated Show resolved Hide resolved
@gabyx
Copy link
Contributor Author

gabyx commented Aug 22, 2022

@imjasonh: I changed the bahv. to warning.

Made a note that we should make it deprecated in v2.0.0. Is there a notes file for v2.0.0 to list all things which might be noeted? Maybe there is also a global GO file to make such a variabel, such that we can add checks, and once we are at 2.0.0 we can set it to true and check all places and refactor them.

@imjasonh
Copy link
Collaborator

Made a note that we should make it deprecated in v2.0.0. Is there a notes file for v2.0.0 to list all things which might be noeted? Maybe there is also a global GO file to make such a variabel, such that we can add checks, and once we are at 2.0.0 we can set it to true and check all places and refactor them.

I don't think we need to wait for a full v2, so long as there's ample time to switch, and clear instructions how. I think that's satisfied here.

If you want to file an issue to remind us all to complete the migration after the next release (v1.10), that sounds fine. Otherwise I think there's enough context in the code and history to come back and remove it later.

Thanks for doing this!

@imjasonh imjasonh merged commit 90e426b into GoogleContainerTools:main Aug 22, 2022
@gabyx gabyx deleted the feature/correct-cli-argument-naming branch August 22, 2022 17:18
@chuangw6 chuangw6 mentioned this pull request Sep 26, 2022
4 tasks
@acohenOT
Copy link

acohenOT commented Oct 8, 2022

README still lists the deprecated parameter. Can this be fixed?
https://github.com/GoogleContainerTools/kaniko#flag---snapshotmode

aazon added a commit to aazon/kaniko that referenced this pull request Mar 14, 2023
Since GoogleContainerTools#2084, the argument `--snapshotMode` has been deprecated in favor of `--snapshot-mode`. The documentation still contains the old names of the argument.
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.

4 participants