-
Notifications
You must be signed in to change notification settings - Fork 704
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
[repository] fixes and enhancements for repository update support #5763
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
75a419a
fixes
dlaloue-vmware c74c978
Merge remote-tracking branch 'origin' into issue5746
dlaloue-vmware d3f3c63
fixes for flux
dlaloue-vmware 7332d4f
carvel repos now allow changing auth type
dlaloue-vmware 5faa8bf
added description support for carvel repositories
dlaloue-vmware e35dd3d
ui fixes related to flux support, plus flux support enhancements
dlaloue-vmware 37d7231
fixes for ui and backend to support auth type change and description …
dlaloue-vmware 26552f4
fixed ui tests
dlaloue-vmware 459dfd3
implemented code review suggestions
dlaloue-vmware 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
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'm just trying to understand some of the logic here: on lines 487 we explicitly disallow the provider from being changed if it has been set, but on line 491 we allow it to be set to empty. So, unless I'm missing something, users can change the auth provider, they just need to first set it to empty then set it?
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.
The idea was that changing the provider did not really make sense (the original code was completely ignoring the new value) so we are explicitly rejecting such change.
The main reason to allow to unset the provider is that the user may have tried with the provider and it did not work and as such is switching back to using basic auth for example.
But yes, a side effect would be that you could unset the provider and switch to another one in the next update.