-
Notifications
You must be signed in to change notification settings - Fork 63
Add extra_values param to app_chart
#665
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
b493e37
add optional token-vendor resources
koonpeng 340ad23
add extra_values option to append to base values.yaml
koonpeng 43e8fc8
Merge remote-tracking branch 'origin/main' into koonpeng/token-vendor…
koonpeng 517cb33
increase default memory request
koonpeng f2f02e9
migrate some usages of values to extra_values
koonpeng 1fd712b
migrate more usages to extra_values
koonpeng 82d5f50
docs
koonpeng a4db766
fix string concat
koonpeng f2648d4
Merge remote-tracking branch 'origin/main' into koonpeng/token-vendor…
koonpeng 8a1d22e
remove values.yaml as it is same as defaults
koonpeng 7760da7
Merge remote-tracking branch 'origin/main' into koonpeng/token-vendor…
koonpeng e44da1f
Merge remote-tracking branch 'origin/main' into koonpeng/token-vendor…
koonpeng a70450b
revert MODULE.bazel.lock
koonpeng a5cea68
Merge remote-tracking branch 'origin/main' into koonpeng/token-vendor…
koonpeng 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 hidden or 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 hidden or 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 hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,3 @@ | ||
| domain: "example.com" | ||
| project: "my-gcp-project" | ||
| registry: "gcr.io/my-gcp-project" | ||
| robots: [] | ||
|
|
||
| udev: | ||
|
|
||
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 was deleted.
Oops, something went wrong.
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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.
TBH. I do not understand we we need 2 values files. If we need to, it should be crystal clear from the code how it would be used and why it is beneficial.
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.
If a values file is not given, a default values file will be used. token-vendor use values in the default values file so if we want to add more values, we need to copy and extend the default. It's just a rare case that if we want to change the default, there is no longer any single SOT.
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 guess Stefan is asking for a cleanup that migrates the other charts to use extra_values. Otherwise people will have the same problem as before: if they copy-paste the example from k8s-relay to a chart that uses values that are not in k8s-relay's values-cloud.yaml, they'll get the same error you did.
Uh oh!
There was an error while loading. Please reload this page.
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.
A cleanup can be in a followup, but I like to have more details on the bazel rule that explains the need.
Eg. when I look at https://github.com/googlecloudrobotics/core/tree/main/src/app_charts/token-vendor
it does not yet have any values.yaml at all.
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.
What is now the usecase for specifying
values? If no rule is doing this anymore and everything is using extra_values, we can keep the attribute name to be called "values" and prepend the defaults?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.
These targets cannot be migrated to use
extra_valuesbecause they override some base values.core/src/app_charts/base/BUILD.bazel
Line 109 in f2648d4
core/src/app_charts/mission-crd/BUILD.bazel
Line 7 in f2648d4
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.
@koonpeng Please decouple the helm_chart change from the token-vendor change: Copy the full list of required values or the subset of required values to token-vendor/values.yaml, and if you still want to make the "create a new values.yaml" flow easier for future developers, create a new PR that adds extra_values.yaml (or just changes the docs to explain where to copy the base values from for full compatibility).
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.
Made another PR that cherry picked #672 the tv changes.
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.
@drigz Now that #672 is merged, this PR should now be a refactor will no functionality changes.