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

add some "extras" tests to exercise the schema a bit more #447

Closed
wants to merge 3 commits into from

Conversation

RangerRick
Copy link
Contributor

No description provided.

@RangerRick RangerRick force-pushed the ranger/big-tests branch 2 times, most recently from f26bc1a to e258cfa Compare December 13, 2024 17:34
@RangerRick RangerRick force-pushed the ranger/big-tests branch 2 times, most recently from 0e5e999 to a9453e1 Compare December 13, 2024 18:31
@RangerRick RangerRick changed the base branch from develop to main December 18, 2024 22:31
Copy link
Member

@LeoColomb LeoColomb 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 to me 👌
One note though, every values.yaml in this ci/ folder adds another ~8 min of for install CI run, and x3 times more on the upgrade CI run.
Might be reasonable to merge this one with an already existing one.

Copy link
Member

@LeoColomb LeoColomb left a comment

Choose a reason for hiding this comment

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

Sorry, actually after re-reading it, I cannot work in the way it is.
See my inline comments.

Comment on lines +5 to +11
- configMap:
name: extraconfig-cm
items: []
optional: true
- secret:
name: extraconfig-secret
secretName: "not so secret"
Copy link
Member

Choose a reason for hiding this comment

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

The indentation was wrong, and I fixed in a previous commit.
Now the actual values/properties are used, and it fails: the mentioned configMap and secret must exist to be referenced and used.
To do this test, we'd need to define extra template with the named configMap and secret.

See

{{- else if $config.configMap }}
configMap:
{{- include "common.tplvalues.render" (dict "value" $config.configMap "context" $) | nindent 4 }}
{{- else if $config.secret }}
secret:
{{- include "common.tplvalues.render" (dict "value" $config.secret "context" $) | nindent 4 }}
{{- end }}

The templating can probably be improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, after figuring out what was wrong in my chart, I'm inclined to just drop this for now. Rye indentation was prettier being a jerk I think?

We should probably have a big values yaml that exercises as much of the chart as possible, given how much time it adds.

@RangerRick RangerRick closed this Dec 18, 2024
@LeoColomb LeoColomb deleted the ranger/big-tests branch December 18, 2024 23:55
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.

2 participants