shipperctl validate chart command #46

[osdrv]

This pull request is aiming to implement a new shipperctl command which will perform a quick probe of a helm chart and report potential issues.

[osdrv]

Hey folks, this PR is now in an i-think-it's-ready-for-a-review state. Please give it a quick look and let me know your impressions @kanatohodets @parhamdoustdar @isutton

[osdrv]

@ksurent I really appreciate your feedback. Very much helpful!

[kanatohodets]

I really dig extracting validation behavior out of the guts of a controller into a library and re-using it from shipperctl.

However, I think this changeset introduces a regression: the old error checks were against the chart as rendered with the values from the Release, while the validation here is against the chart as rendered with the default values.

[kanatohodets]

I think we need to pass rel.Spec.Environment.Values into Validate.

Helm Charts can have conditional branches based on values, which might cause the Chart to render completely differently with and without a particular value. In the extreme case, you can consider something like:

{{- if .Values.injectDebugService -}}                                                                                                                                              
apiVersion: v1
kind: Service
metadata:
  name: my-debug-service
spec:
  type: ClusterIP
  ports:
  - name: web
    port: 80
    targetPort: 8888
  selector:
    mode: debug
{{- end -}} 

where a Service object is only rendered when someone has enabled the 'debug' value on the Chart. This might be the difference between Shipper finding a Chart valid or not.

[kanatohodets]

Thinking about having values be a param, I might do the same for renderManifests: have it take name, namespace, and values as arguments instead of inferring them from the associated Release.

The reason is that eventually we want to change the InstallationTarget objects to be independently usable, without requiring an associated Release, as per #5. This is purely a target of opportunity though, so if it looks like a gnarly change, totally fine to skip.

[kanatohodets]

If we change Validate to take values as an argument, we'll need to come up with the UI for users to specify those values. Probably the most intuitive way would be to do it exactly like helm, with --set foo=bar or -f values.yaml, but neither of those arguments really make sense in a Shipper world: you never use them when you're using Shipper, you just write your app.yaml.

I could imagine doing something like -f app.yaml instead. However, then you have a full Application object which specifies its own Chart, which makes it odd to validate only the Chart bit -- especially if you're validating a Chart that is different from the one specified in the Application. Most likely don't want to validate a Chart independent of an Application; you should instead validate the entire Application.

In that case the command would be better expressed as something like shipperctl validate app -f app.yaml, and part of doing so is downloading/validating the Chart. What do you think?

[luisdavim]

I think that both validations are useful, I would want to validate that the app.yaml and the charts referenced in it are ok but I would also a quick way to check if an existing chart would work with shipper, in the second case using the default values should be ok.

[kanatohodets]

Build should start working once rebased on master (codegen needed updating)

[osdrv]

Closing this PR due to an uncertainty regarding it's applicability and usability.