Skip to content
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

User defined templates #715

Merged
merged 22 commits into from
Feb 19, 2025

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Feb 6, 2025

This PR attempts to resolve #713

There are now two supported options.

  1. Add overriding files to the ConfigMap for the deployment and the other requires users to

  2. Add additional build step in order to add their own html template directory to the controller image and change the environment variable CONTROLLER_TEMPLATE_DIR to point to their new template directory.

Why 2 Options
While option 1 prevents the need for additional build steps it is limited by the maximum size of a ConfigMap (1MB). Option 2 allows for more complex user made html templates but requires an additional build step.

@coveralls
Copy link

coveralls commented Feb 6, 2025

Pull Request Test Coverage Report for Build 13422494467

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 85.272%

Totals Coverage Status
Change from base Build 13402019810: 0.02%
Covered Lines: 689
Relevant Lines: 808

💛 - Coveralls

Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
@Gavinok Gavinok requested a review from loneil February 7, 2025 17:35
Gavinok and others added 4 commits February 7, 2025 09:35
@Gavinok Gavinok requested a review from esune February 7, 2025 18:31
Copy link
Contributor

@loneil loneil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what people think about default location?
For the override variables it's CONTROLLER_VARIABLE_SUBSTITUTION_OVERRIDE=/etc/controller-config/user_variable_substitution.py right? Good to have templates in the same spot rather than /tmp? Maybe we add more config files later on so could be good to have things together...

I haven't tried an override locally in docker (just confirmed it runs all good as-is), but I think the solution should work (would get Ivan to give a look through Helm setup on this PR) if I'm understanding right. So anything I put in /tmp/templates (or whatever we specify that to be, or the user species with CONTROLLER_TEMPLATE_DIR), that should get resolved at runtime and pulled in right?
So it's not necessary to build the image with custom templates? Like if someone adds a volume mount at their template directory could those files be independently controlled as deployment time?

Ideally we (and others) wouldn't be building custom I think, just using the vcauth image and deploying files and setting the path? If I'm reading the config right.

docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like @loneil suggestion to keep all templates together in /etc/controller-config/ as default directory. We can still provide a way to specify a different location, but that path would make sense in most scenarios.

I am also wondering if we should put a "toggle" for the template configmap to be mounted or not (it would always be created in k8s): if we don't do this, even when using a custom build image we would get the configmap mounted at the target location overriding the modified files.

docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
@Gavinok Gavinok requested review from esune and loneil February 10, 2025 20:04
@Gavinok
Copy link
Contributor Author

Gavinok commented Feb 10, 2025

Resolved the grammar and corrected the directory + registry address. Do we also want to change the chart to point at that registry?

@esune
Copy link
Member

esune commented Feb 11, 2025

Resolved the grammar and corrected the directory + registry address. Do we also want to change the chart to point at that registry?

Resolved the grammar and corrected the directory + registry address. Do we also want to change the chart to point at that registry?

Yes please, that was something I must have missed when prepping for the move

Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
esune
esune previously approved these changes Feb 11, 2025
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍🏻

Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more tweaks, sorry for the back-and-forth: after reviewing the current strategy with @i5okie he came up with some extra recommendations that are worth following.

charts/vc-authn-oidc/values.yaml Outdated Show resolved Hide resolved
charts/vc-authn-oidc/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/vc-authn-oidc/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/vc-authn-oidc/templates/configmap.yaml Outdated Show resolved Hide resolved
docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
@i5okie
Copy link
Contributor

i5okie commented Feb 13, 2025

I just created an issue regarding placing files in /etc directory. #721
In the context of this PR, I think placing these templates in /app/templates would be more appropriate.

@Gavinok
Copy link
Contributor Author

Gavinok commented Feb 18, 2025

I have updated this to reflect the requests from both @i5okie and @esune . Let me know if there is anything else

@Gavinok Gavinok requested a review from esune February 18, 2025 21:20
Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple nitpick suggestions for the docs and we're good to go I think.

docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
docs/ConfigurationGuide.md Outdated Show resolved Hide resolved
@esune esune merged commit 1f88935 into openwallet-foundation:main Feb 19, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customization of QR code page
5 participants