-
Notifications
You must be signed in to change notification settings - Fork 74
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
Reorg config files #685
Reorg config files #685
Conversation
89de0bb
to
3d3dc24
Compare
charts/vc-authn-oidc/values.yaml
Outdated
userVariableSubsitution: |- | ||
# This is a default placeholder Python file | ||
# Add your default content here |
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.
Default "blank" file here
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.
Looks good. Do we want to have a commented-out example of a substitution?
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.
Oh yeah, there's the example file in the source here
https://github.com/bcgov/vc-authn-oidc/blob/main/docker/oidc-controller/config/user_variable_substitution_example.py
so could add a function from that commented out, or just a comment saying they could look at that file
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.
My vote goes to adding a function/example for that file commented-out to make it foolproof.
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 am tempted to say the name but also it makes it far harder to keep the examples all up to date if we chose to update the API. Just a thought to consider but chances are the API won't change any time soon
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.
Added commented out file
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.
lgtm
f1df40e
to
e068d17
Compare
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.
Looks good, I think if we add the default example code to the values file for the variable substitution we can merge and test in OCP
Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
330abac
to
0ea0e8e
Compare
Fix (hopefully) issue with VCAuthN deployment from missing config map. Reorg things so we're just using a single config map instead of intending on one per configuration file. Renamed it "controller-config" instead of specific to "session timeout". Add the "blank" (comments, see below) placeholder user variable sub file to the config map.
Default population for this user variable py file comes from values. So keep the pattern of being able to override it with a values yaml, or operator can have whatever means up updating configmap, or the volume itself, as works for their CICD.
App continues to use environment variable to know which path to look for these files, so can override that as well.
Discussed moving location of files to /app but thinking about that didn't want to volume mount into the directory the code goes into, as that could cause confusion (and if you can alter /app it's like changing the source code anyway instead of "external" configuration), and to keep /app as deployed, built source alone.
Used
/etc/controller-config
instead of/home/aries
Don't really care about /etc vs /home but googled practices for config file stuff
Since this is a system service thing and not a user preference...🤷 again not really important. Important part is to not use "aries" naming any more if we can avoid it. If there's any weird permission thing, or if people think
/home
is a more obvious place to look can change it.Tested helm install locally on minikube and things seem to be where they're expected (below). But will need to make sure OCP deployment is all good and the Python app doesn't have issues loading.
If we merge this I'll follow through on Dev env and make sure things are deploying ok and the app is picking up the values as expected.
New config map, contains both default file values:
Checking on controller pod: