-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix: argocd version --client tries to connect server #8341
Merged
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6c957f0
fixed issue with version --client cmd
gdsoumya 3ee3065
moved local api server creation to NewClientOrDie()
gdsoumya 873ee06
lint fixes
gdsoumya e36bf7a
revert makefile change
gdsoumya 7d61c55
fixed go imports order
gdsoumya 49b01b5
revert makefile change
gdsoumya f5a717b
lint fix
gdsoumya 8977ba2
reverted generated file changes
gdsoumya 62937ca
fix: moved headless to cli
gdsoumya 2db0076
refactor: reverted back original NewCLientOrDie
gdsoumya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this a little bit cleaner. Everywhere we repeat a call to
initialize.RetrieveContextIfChanged
when initializing the client. i.e.:Can we somehow improve/reduce this to:
Can we update the clientOpts to have a
.context
field so we don't have to parse this everywhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was initially thinking of doing that but this specific context is only required when we need to create the local server and the CLientOptions struct is a more general client pkg data that might be used by other API clients and binaries so I felt it might be better to keep it specific to the cli only instead of modifying the original struct. But we can add it in there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point. But unfortunately, there's already a
clientOpts.Core
which is a useless flag to the generic argocd client package. So for better or worse, we already have CLI specific flag which made its way into inclientOpts
.If we want to be slightly cleaner about this, we could preserve the existing API client constructor. e.g.:
And introduce a CLI specific constructor that accepts the
cobra.Command
as a second argument:The
headless.NewClientOrDie
would call:Then we don't have to introduce more useless options to
clientOpts
which aren't really used by the API client package. And we could even remove the Core option from clientOpts. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me, also we'll shift the
headless.NewClientOrDie
to the CLI pkg so it will not technically change anything in the apiclient pkg.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed