-
Notifications
You must be signed in to change notification settings - Fork 19
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 Helm manifest gen #447
Conversation
1a50ea3
to
97bc62a
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.
It's awesome that you added test coverage and refactored code, it's more readable right now.
Regarding problem with escaping in jinja templating we can wait for our jinja sensei @lukaszo
internal/cli/alpha/manifestgen/templates/helm-implementation.yaml.tmpl
Outdated
Show resolved
Hide resolved
I reopened the PR in #459, as I accidentally removed the branch from my fork... edit: nvm |
@Trojan295 FYI you could reopen the same pull request if you didn't create the new one 🙂 You can try closing the new one (#459) and reopening this, probably it would be easier to track post-review changes, but it's up to you. |
internal/cli/alpha/manifestgen/templates/helm-implementation.yaml.tmpl
Outdated
Show resolved
Hide resolved
2c0b807
to
68bd7c5
Compare
68bd7c5
to
bc3deeb
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.
lgtm, just minor comments
} | ||
|
||
if len(parentKeyPath) > 0 { | ||
schema.Title = parentKeyPath[len(parentKeyPath)-1] |
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.
nit
schema.Title = parentKeyPath[len(parentKeyPath)-1] | |
schema.Title = strcase.ToCamel(parentKeyPath[len(parentKeyPath)-1]) |
arguments: | ||
artifacts: | ||
- name: input-parameters | ||
from: "{{"{{"}}steps.prepare-parameters.outputs.artifacts.merged{{"}}"}}" |
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.
from: "{{"{{"}}steps.prepare-parameters.outputs.artifacts.merged{{"}}"}}" | |
from: "{{`{{steps.prepare-parameters.outputs.artifacts.merged}}`}}" |
IMO it can be done in more readable way, simply replace
"{{"{{"}}
with
"{{`{{
(in the same way right brackets)
so from this
"{{"{{"}}steps.prepare-parameters.outputs.artifacts.merged{{"}}"}}"
you will get
"{{`{{steps.prepare-parameters.outputs.artifacts.merged}}`}}"
* solves problem nr 2 from Add Helm manifest gen #447 * add unittests * complete features list * enable running python tests on CI
Description
Changes proposed in this pull request:
capact alpha manifest-gen implementation helm
command--override
flag to--overwrite
Testing
You can use the following command to generate manifests for the
bitnami/postgresql
Helm chart:go run cmd/cli/main.go alpha manifest-gen implementation helm cap.implementation.bitnami.postgresql.install postgresql --repo "https://charts.bitnami.com/bitnami" -i cap.interface.database.postgresql.install
Problems
If user provides an empty parameter value for a string, it will be interpreted as null, when evaluating values in workflow.
The values of the parameters are transformed to strings in format:
I couldn't find a way to make
yaml.Marshal
add double quotes to every string value so it would be ok.If a key in Helm values.yaml contains a dot, then Jinja will have problem with interpreting it correctly. I'm not sure if this is a problem in Jinja as changing it to
object[key]
instead ofobject.key
does not work also.Related issue(s)
Closes #417