Add nodejs helm chart

[AshCripps]

This is a draft pr for adding a helm chart for nodejs in a similar style to the quarkus one

This is still a WIP and things like the NOTES.txt aren't updated yet but wanted to open the PR to get feedback that im going in the right direction.

The next question I have is how to handle the jvm vs native mode in this chart compared to the quarkus one - atm I only have the native functionality

[mhdawson]

Another thing I noticed is that nodejs-ex-k has a values.schema.json which lets it play nicely with the openshift ui. The Quarkus one does not seem to have that so you end up with only the YAML versus the FormView.

This is the form for nodejs-ex-k

The net is that we should include a values.schema.json to make it play nicely with the OpenShift UI. This may also explain why it does not seem to have the typical templating seen in generated helm chart files. Maybe the the UI is doing a substitution into the default files versus using the standard helm substitution?

[mhdawson]

As mentioned, we are not going to land/move this forward until we've had a broader discussion with the openshift/helm team on the bigger picture strategy. Naina is going to help queue up those discussions early in Jan.

[sabre1041]

@AshCripps Wanted to follow up if you have had a chance to revisit this PR

cc: @mhdawson

[mhdawson]

@sabre1041 Naina (the Node.js PM) was going to set something us to discuss across languages. I've not seen that yet but I'll follow up with her to see when that might happen.

[mhdawson]

@AshCripps let me know when it's ready for me to take another look as well.

[AshCripps]

@mhdawson should be ready now

[mhdawson]

@AshCripps did you managed to deploy to a cluster where you could try out the UI deploy? If so can you outline the steps of what was necessary?

Overall I think it looks good. Left a few minor comments/questions but assuming we've tested it out in the UI, from the command UI and your changes addressed @deweya's request it should be good to go.

[deweya]

I see. The image.name and image.tag should be included in values.schema.json

I do think build is a necessary feature of the chart.

I think I misspoke in my previous comment. The common use case as I see it is to leave the image values as their default and leverage the build capability

Sent from my iPhone

On Feb 4, 2021, at 3:46 PM, Michael Dawson notifications@github.com wrote:

 @mhdawson commented on this pull request.

In alpha/nodejs-chart/values.schema.json:

• "default": "TCP"
• }
• }
• },
• "build": {
• "type": "object",
• "title": "Build",
• "description": "Specify what repo to build from",
• "required": [
• "enabled",
• "uri",
• "ref"
• ],
• "additionalProperties": true,
• "properties": {
• "enabled": { My comments were around the UI, the alpha/nodejs-chart/values.schema.json defines the elements that the user can configure through the form. When I say dropping down you YAML I meant having to switch from the form view to the YAML editor to directly edit the YAML instead of using the form. In terms of what you mentioned, the form does not currently expose "image.name and image.tag" which was the reason form my original comment, if you want to edit those you have to switch the YAML editor. My question was then if you have to switch to the YAML editor for the common use case, does it make sense to expose build/no build in the form.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[deweya]

The common use case I was describing previously was if users didn’t want to use the build feature, in which case I would assume the reason would be because they want to pull a docker image. Then they would have to set the image values.

Either way I think the image.name and image.tag definitely need to be included in the UI.

Sent from my iPhone

On Feb 4, 2021, at 7:46 PM, Austin Dewey deweya964@gmail.com wrote:

I see. The image.name and image.tag should be included in values.schema.json

I do think build is a necessary feature of the chart.

I think I misspoke in my previous comment. The common use case as I see it is to leave the image values as their default and leverage the build capability

Sent from my iPhone

On Feb 4, 2021, at 3:46 PM, Michael Dawson notifications@github.com wrote:

 @mhdawson commented on this pull request.

In alpha/nodejs-chart/values.schema.json:

• "default": "TCP"
• }
• }
• },
• "build": {
• "type": "object",
• "title": "Build",
• "description": "Specify what repo to build from",
• "required": [
• "enabled",
• "uri",
• "ref"
• ],
• "additionalProperties": true,
• "properties": {
• "enabled": { My comments were around the UI, the alpha/nodejs-chart/values.schema.json defines the elements that the user can configure through the form. When I say dropping down you YAML I meant having to switch from the form view to the YAML editor to directly edit the YAML instead of using the form. In terms of what you mentioned, the form does not currently expose "image.name and image.tag" which was the reason form my original comment, if you want to edit those you have to switch the YAML editor. My question was then if you have to switch to the YAML editor for the common use case, does it make sense to expose build/no build in the form.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[mhdawson]

@deweya, thanks for the clarification. With all of that context seems like we both agree that we should add image.name and image.tag to the UI. @AshCripps is that something you can do?

[AshCripps]

@deweya, thanks for the clarification. With all of that context seems like we both agree that we should add image.name and image.tag to the UI. @AshCripps is that something you can do?

already have

[mhdawson]

@AshCripps thanks.

[AshCripps]

@sabre1041 @deweya im unable to test this chart in the UI - it just doesn't appear when my add my fork as a helm chart repository in openshift. Do I need to package it up as a tar before I can get it to appear in openshift?

[deweya]

@AshCripps Yeah, you need to package the chart first in order to see it from the UI

[sabre1041]

@AshCripps If you publish the chart and expose the index.yaml file, you can then add a HelmRepository resource

[AshCripps]

@sabre1041 @deweya Thanks! managed to get it to appear in the UI now - but am now facing this error

Invalid root object field configuration:uiSchema order list does not contain property 'global'.

Just wanted to ask if you could spot any obvious error before I dig into this again next week

[AshCripps]

@deweya @sabre1041 @mhdawson Ive updated the values.schema.json and managed to test it in the UI and its all working for me so this PR should be good to go now

[mhdawson]

@AshCripps from my perspective it is good to go then. Separately could you email me the instructions on what you had to do in order to test it in the UI as I'd like to know how to do that.

[deweya]

Hey @AshCripps ,

Just tried to install the chart in OpenTLC and got this error when installing with the default values:

→ oc logs bc/nodejs-test --follow
Cloning "https://github.com/nodeshift-starters/nodejs-rest-http" ...
error: fatal: Couldn't find remote ref main
Unexpected end of command stream

This is because https://github.com/nodeshift-starters/nodejs-rest-http has a master branch, but not main. Changing to master built properly.

I'd like our charts to try and use main as the default since that is the new GH standard, but I get that this example uses master. Maybe for this chart we use master as the default build.ref since that would be the correct default value for the example?

[AshCripps]

@deweya we have a task to change our repos to main and I did raise it again this week, but I'll ask someone in my team who has the required permission to change it tomorrow.

[deweya]

@AshCripps Perfect!

[mhdawson]

@AshCripps great I was going to suggest we expedite changing versus changing this chart.

[AshCripps]

@deweya The repo should now have main as its default

[mhdawson]

@deweya, @sabre1041 this is just waiting your reviews to land.

[deweya]

@sabre1041 any feedback on your end?

[AshCripps]

Comments addressed

[AshCripps]

@sabre1041 can you rereview?

[sabre1041]

LGTM!