-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update CRD with meaningful set of parameters #56
Merged
rm3l
merged 31 commits into
janus-idp:main
from
rm3l:26-update-crd-with-initial-set-of-meaningful-parameters
Dec 12, 2023
Merged
Update CRD with meaningful set of parameters #56
rm3l
merged 31 commits into
janus-idp:main
from
rm3l:26-update-crd-with-initial-set-of-meaningful-parameters
Dec 12, 2023
Conversation
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
gazarenkov
requested changes
Dec 4, 2023
rm3l
changed the title
[WIP] Update CRD with meaningful set of parameters
Update CRD with meaningful set of parameters
Dec 4, 2023
rm3l
changed the title
Update CRD with meaningful set of parameters
[WIP] Update CRD with meaningful set of parameters
Dec 8, 2023
We discussed and agreed on the CRD structure: doc Summarized in #26 (comment) I'm updating this PR with the new changes.. |
…fig and extra config files
Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com>
…or ephemeral containers Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com>
Co-authored-by: Jianrong Zhang <jianrzha@redhat.com>
Co-authored-by: Jianrong Zhang <jianrzha@redhat.com>
…cal DB Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com>
Now that we have support for environment variables and mounting extra files, we can simplify the logic by not allowing using Secrets. This is also more aligned with the Backstage way of writing configuration files [1] [1] https://backstage.io/docs/conf/writing/ Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com>
…nfiguration Now that we have support for environment variables and mounting extra files, we can simplify the logic by not allowing using Secrets. This is also more aligned with the Backstage way of writing configuration files [1] [1] https://backstage.io/docs/conf/writing/ Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com>
This makes it more aligned with the other fields
This makes it more aligned with how references to secret keys are named generally in K8s
Co-authored-by: Jianrong Zhang <jianrzha@redhat.com>
[1] #26 (comment) Co-authored-by: Gennady Azarenkov <gazarenkov@gmail.com> Co-authored-by: Jianrong Zhang <jianrzha@redhat.com>
This sounds too low-level and might sound confusing. Instead, if no AppConfig, ExtraFile or ExtraEnvVar is set, we would create a default app-config with a default value for this backend auth secret. Otherwise, we would make it clear in the doc that users need to additional define this in their own app-config or extra-env vars.
rm3l
changed the title
[WIP] Update CRD with meaningful set of parameters
Update CRD with meaningful set of parameters
Dec 11, 2023
We use pointers generally to be able to determine if a field was purposely not set. But it should not matter here as the default value is 'false'.
gazarenkov
approved these changes
Dec 11, 2023
2 tasks
Co-authored-by: Jianrong Zhang <jianrzha@redhat.com>
gazarenkov
approved these changes
Dec 12, 2023
rm3l
deleted the
26-update-crd-with-initial-set-of-meaningful-parameters
branch
December 12, 2023 10:57
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This updates the CRD structure based on the proposal and discussion in #21 (comment), later refined in https://docs.google.com/document/d/14z9tE2XsCRl0hLTbHkyOdv26S6trHLgfpxYOfo-cm1I/edit
This is an example of what we agreed to have:
More context in:
Which issue(s) does this PR fix or relate to
PR acceptance criteria
How to test changes / Special notes to the reviewer
The most basic CR (with no spec) should work.
See examples of more complete CRs here:
janus-cr.yaml
rhdh-cr.yaml
Noteworthy changes:
/opt/app-root/src
by default)/opt/app-root/src
by default)dynamicPluginsConfigMapName
string field.backendAuthSecretRef
field introduced in Handle app-configs and config for dynamic plugins in Custom Resource #27 has been removed to make it less confusing. Instead, if user did not specify anyappConfig
,extraFiles
orextraEnv
in their CR, we would create a default app-config with a default value for the backend auth key. Otherwise, users are expected to provide their own value for this key (either via their own app-config, or via anAPP_CONFIG_...
env var)