feat(synth): option to specify chart name for helm format #3458
Conversation
MatthewCane
changed the title
Head branch was pushed to by a user without write access
MatthewCane
force-pushed
the
allow-custom-chart-name
branch
3 times, most recently
from
217ff0f to
3ba25da
Compare
|
@iliapolo could I get some feedback on this please? Thanks! |
iliapolo
changed the title
--chart-name option to format is helm
iliapolo
changed the title
--chart-name option to format is helm
| exports[`Helm synthesis --chart-name is used when specified with all inputs from cli and no config file is present 1`] = ` | ||
| Object { | ||
| "Chart.yaml": "apiVersion: \\"v2\\" | ||
| name: \\"cdk8s-synth-test\\" |
There was a problem hiding this comment.
I believe this isn't the expected value here - it should be custom-chart-name?
Its because you need to add chartName here:
cdk8s-cli/test/synth/synth-stdout.test.ts
Line 1088 in 9f54e63
Otherwise, the cli input tests don't actually pass it to the synth handler.
There was a problem hiding this comment.
Good catch, thank you for the feedback! I have added that now and pushed the change.
test/synth/synth-stdout.test.ts
Outdated
| synthConfig: { | ||
| format: SynthesisFormat.HELM, | ||
| chartVersion: '1.1.1', | ||
| chartName: 'custom-chart-name', |
There was a problem hiding this comment.
Dont think this belongs here - this test says its checking for: "default chart name is used when --chart-name is not specified %s".
So why add it?
Note that my previous comment was about adding chartName here:
cdk8s-cli/test/synth/synth-stdout.test.ts
Line 1088 in ac7899a
So:
chartApiVersion: options.chartApiVersion,
chartVersion: options.chartVersion,
+ chartName: options.chartNameThere was a problem hiding this comment.
It's been a while since I could look at this, does it look better now?
Signed-off-by: Matthew Cane <matthew.cane@citizensadvice.org.uk>
Signed-off-by: Matthew Cane <matthew.cane@citizensadvice.org.uk>
MatthewCane
force-pushed
the
allow-custom-chart-name
branch
from
259ae18 to
c675990
Compare
Signed-off-by: Matthew Cane <matthew.cane@citizensadvice.org.uk>
MatthewCane
force-pushed
the
allow-custom-chart-name
branch
from
c675990 to
b7e8210
Compare
|
This PR has not received a response in a while and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer. |
github-actions
bot
added
the
closing-soon
MatthewCane
force-pushed
the
allow-custom-chart-name
branch
from
a33536a to
f9fdd40
Compare
|
@iliapolo I have pushed an update, sorry for the delay, I've not been able to look at this for a while. |
github-actions
bot
removed
closing-soon
Currently, when synthesising an app as a Helm chart the chart name is the base path of the chart.
This presents an issue when a cdk8s application is not in the root of the project, for example if the application is located in
/infrastructure/chartwithin a larger project, the name of the chart will just bechart. This could be confusing.This change adds an optional parameter
--chart-namein the CLI orchartNamein the config file that will allow users to override this value.I have added tests which pass and tested locally and it works as expected.
This is my first PR for cdk8s so I am open to suggestions for improvements.