-
Notifications
You must be signed in to change notification settings - Fork 20
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
RHCLOUD-35503 updates defaults and adds config pkg #176
RHCLOUD-35503 updates defaults and adds config pkg #176
Conversation
0d5dca0
to
4f4814e
Compare
found some minor issues that need to be fixed before merging |
4f4814e
to
f8d5a3c
Compare
AllowAll = "allow-all" | ||
Kessel = "kessel" | ||
AllowAll = "allow-all" | ||
Kessel = "kessel" |
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.
Here Kessel
Refers to relations-api
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 reason for adding the RelationsAPI = "kessel-relations"
const is just so that i can reference that specific value HERE instead of hardcording "kessel-relations" in that method as this is the expected name of the relations ClowdApp that shows up in appconfig. This way if it ever changes, we just change the const
Similar to how the authz.impl config has a Kessel const which i use to set the impl value to 'kessel' in
inventory-api/internal/config/config.go
Line 82 in f8d5a3c
o.Authz.Authz = authz.Kessel |
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 would suggest changing the Kessel
to RelationsAPI
. authz.Kessel and RelationsAPI both are referring to "kessel-relations"
PR set back to ready, validated the changes i made are still working in ephemeral. |
PR Template:
Describe your changes
NewOptions
calls to production like settings, reducing the number of settings that need explicit defining in config files or startup commandsoptionalDependency
of relations-api to capture config dataenable-oidc-auth
setting as it was invalidNote: I also tested local deployment methods (docker compose, make run) and these changes did not seem to impact any of it
This has also been tested in ephemeral
Validation:
Some Notes:
Config File --> CLI flags/explicit option setting (what the ClowderInjection code does) --> Default options
Example:
authz.kessel.insecure-client
is set by default as false in default optionsauthz.kessel.insecure-client
is set to true in the inventory configmap in deployment yamlThe insecure setting from the config overrides the default flag or any explicit option settings
This means anything we wish to override can be set at config file without code changes and it will take precedence
Ticket reference (if applicable)
For RHCLOUD-35503
Checklist
Are the agreed upon acceptance criteria fulfilled?
Was the 4-eye-principle applied? (async PR review, pairing, ensembling)
Do your changes have passing automated tests and sufficient observability?
Are the work steps you introduced repeatable by others, either through automation or documentation?
The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)
Are the agreed upon coding/architectural practices applied?
Are security needs fullfilled? (e.g. no internal URL)
Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)
For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?