-
Notifications
You must be signed in to change notification settings - Fork 194
Added Joget package #1110
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
Added Joget package #1110
Conversation
adamkpickering
left a comment
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.
Looks good, just a couple of minor comments.
packages/joget/joget/upstream.yaml
Outdated
| Vendor: joget | ||
| DisplayName: Joget DX | ||
| ChartMetadata: | ||
| kubeVersion: '>=1.21-0' No newline at end of file |
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 is best not to set kubeVersion in upstream.yaml. If the upstream chart sets kubeVersion to something other than what you have here, this will still override it, even if it is now wrong. Yes, it would be possible to remember to change this override when that happens, but in practice it will get forgotten.
If the upstream value is wrong it should be fixed in the upstream chart. Is this not possible?
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.
Yes, it is possible. Will remove it kubeVersion from upstream.yaml
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.
Removed the kubeVersion
|
Updated the PR: Removed |
packages/joget/joget/upstream.yaml
Outdated
| @@ -0,0 +1,4 @@ | |||
| HelmRepo: https://jogetworkflow.github.io/helm-joget/charts | |||
| HelmChart: joget | |||
| Vendor: joget | |||
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.
Do you want to display Joget (i.e. with a capital J) as the vendor name in the Rancher UI? If so, you should have Vendor: Joget. Otherwise there is no point in setting Vendor: joget, since joget can be gotten from the package path.
|
Updated vendor in |
bdd8e15 to
42420b4
Compare

Added Joget package