minimal implementation for imagePullSecrets

[gprossliner]

This is the very minimal implementation. Some notes:

• I can't use a default value for the imagePullSecrets in values.yaml, as this should only be specified by the user if necessary. To let the user know about the feature, I render a comment in values.yaml with an example how to use it.
• I use toJson in the helm expression to get started somewhere
• There could be some refactoring done generally to handle PodSpec uniformly in one place. Currently there is quite a few duplicate code in daemonset.go and deployment.go. For this PR I just kept it like it is, and implemented in handling in an own package
• I tested the generate e2e app chart, and it looked good at the first glace.

Please let me know what you think about it.

[arttor]

What do you think about this approach:

1. parse deployment template spec (we can move the processor to a separate func or do copypaste for now)
2. if spec.imagePullSecrets is not set, then:
3. add <deployment>.imagePullSecrets: "" to values.yaml (we already moving <deployment>.<container>.image there)
4. add this to deployment helm template:

   spec:
   {{- with .Values.imagePullSecrets }}
     imagePullSecrets:
       {{- toYaml . | nindent 8 }}
   {{- end }}
     containers:

PS I like your approach and understand that it will be hard to add {- with to template because it is not just a value in the spec map. But 2 moments bothering me:

• if user provides a manifest without imagePullSecrets, helmify will generate manifest with imagePullSecrets in spec.
• i think that controlling helm output by setting empty value in values.yaml is better than uncommenting values in it.

[gprossliner]

Thank you for the review. As you have noticed, adding {- with would need some general code refactoring / rework, because you can't serialize it from the map, and I see no other block statements currently emitted in the templates, maybe because of the same reason.

Does it really matter? The templates are not supposed to be edited manually, so IMHO it's about the behaviour.

if user provides a manifest without imagePullSecrets, helmify will generate manifest with imagePullSecrets in spec.

With the current expression, and without providing imagePullSecrets in the values, this PR renders just an empty array, which behaves the same as if it has not been specified at all. We could also yield to null if preferred.

i think that controlling helm output by setting empty value in values.yaml is better than uncommenting values in it.

If I understood you correct, you meant to omit the comment, and just mention the feature in readme?

We could also emit imagePullSecrets: [] in values.yaml. I don't quite get your proposed imagePullSecrets: "", because it's expected to be an array.

Do you think we should scope the imagePullSecrets within the deployment / daemonset, or leave it in the global scope?

[arttor]

Yep, i think it will be better to add an empty value to values.yaml instead of comments. Can you please also add imagePullSecrets to one of the deployments in https://github.com/arttor/helmify/blob/main/test_data/sample-app.yaml and regenerate samples in https://github.com/arttor/helmify/tree/main/examples in your MR?

[gprossliner]

I removed the comment. Now an empty array is emitted in values.yaml. I also regenerated the sample app.

[arttor]

sorry, one more thing. what do you think about adding flag --image-pull-secrets for this feature and disabling it by default?

[gprossliner]

Sure I can do this if desired. Beside the --crd-dir there is currently no other flag to enable / disable features. IMO this would be better contained within a configuration file. But currently there is no configuration (other PRs also mention this), so if this feature should be opt-in, this is the only sane way to implement this.

[arttor]

Absolutely agree that we should have a config file but even with a config file, it is good to have flags. I use helmify in k8s operator framework which in turn uses make. So for k8s operators it is easier to add flags in make than adding config file to env. Please add flag similar to --crd-dir. In this case you can move feature documentation from readme to --help command.

[gprossliner]

Have added the flag.

BTW I use it for an operator too :-)

[gprossliner]

I refactored the flag into the config.Config struct like other flags.

Should I regenerate the demo app without the flag been enabled?

[arttor]

no need to regenerate. just fix review issues and we can merge it.

[gprossliner]

HI arttor. Hope you had a nice Sunday. I think it's done now. Thank you!

[arttor]

Sorry, i still can see ## ImagePullSecrets heading in readme

[gprossliner]

Ok. I didn't get this. I thought about adding it in tag help too. Will update iit!

[gprossliner]

Thank you for merging. I'm awaiting the release!