Skip to content
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

Docker plain helm chart #1035

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

Docker plain helm chart #1035

wants to merge 40 commits into from

Conversation

segfault16
Copy link

Add helm chart for docker plain

Refers to #1013

Tasks:

  • Updated documentation in docs/modules/... directory
  • Ran tests in <quickstarter>/testdata directory
  • Shared library adaptions

@BraisVQ
Copy link
Contributor

BraisVQ commented Sep 5, 2024

The base image has been changed to nginxinc/nginx-unprivileged because the alpine one did not work properly with the container probes

@gerardcl
Copy link
Member

gerardcl commented Sep 5, 2024

The base image has been changed to nginxinc/nginx-unprivileged because the alpine one did not work properly with the container probes

and this also now does match with the values.yaml image section, perfect!

@gerardcl gerardcl requested a review from renedupont September 5, 2024 17:16
@BraisVQ
Copy link
Contributor

BraisVQ commented Sep 6, 2024

Provision, build, and release pipelines have been tested and works out of the box with the latest commits.

@@ -0,0 +1,41 @@
# TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case we forget, this file should be removed before the PR is merged

- Review helm install command
- Release manager image checks
- Agree if helm install requires overwritting values on image (see helm notes and deployment, and values.yaml)
- provide docs on how to test chart updates, by running `helm --debug template . ` under the chart folder to be tested
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also helm lint

kind load docker-image testing/my-component:$(git rev-parse --short=8 HEAD)
helm upgrade --install --wait --atomic --namespace docker-plain --set image.path=testing --set image.name=my-component --set image.tag=$(git rev-parse --short=8 HEAD) docker-plain chart
```

## How to create a custom jenkins-agent out of this docker-plain component
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needed?

# - yourapp.yourdomain.com

service:
enabled: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service.enabled is never used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with commit fe5fa7f

@faust2199
Copy link
Contributor

Having a very strict values.schema.json is probably not going to provide good experience when using helmValues defined from the Jenkinsfile. According to https://helm.sh/docs/topics/charts/ , "the schema is applied to the final .Values object, and not just to the values.yaml file."

Let's say we have the following in the Jenkinsfile:
odsComponentStageRolloutOpenShiftDeployment(context, [ 'selector': "app.kubernetes.io/name=${context.componentId}", 'helmEnvBasedValuesFiles': ['values.env.yaml'], 'helmValues': [ 'foo': 'bar' ] ]

When the above runs on Jenkins, ods-jenkins-shared-library will add two arguments on the command line:
--set foo=bar --set global.foo=bar

Then it will be aborted because additionalProperties are not allowed in both places. Is the user really expected to add two entries in values.schema.json to fix this?

I suggest that we remove the whole values.schema.json file. If the result of rendering is bad, helm upgrade is going to fail and rollback anyway.

@renedupont
Copy link
Member

Having a very strict values.schema.json is probably not going to provide good experience when using helmValues defined from the Jenkinsfile. According to https://helm.sh/docs/topics/charts/ , "the schema is applied to the final .Values object, and not just to the values.yaml file."

Let's say we have the following in the Jenkinsfile: odsComponentStageRolloutOpenShiftDeployment(context, [ 'selector': "app.kubernetes.io/name=${context.componentId}", 'helmEnvBasedValuesFiles': ['values.env.yaml'], 'helmValues': [ 'foo': 'bar' ] ]

When the above runs on Jenkins, ods-jenkins-shared-library will add two arguments on the command line: --set foo=bar --set global.foo=bar

Then it will be aborted because additionalProperties are not allowed in both places. Is the user really expected to add two entries in values.schema.json to fix this?

I suggest that we remove the whole values.schema.json file. If the result of rendering is bad, helm upgrade is going to fail and rollback anyway.

I partially agree, I think there is a middle way though. In the values.schema.json we could set "additionalProperties": false to true in the relevant parts. That way we validate everything that is set in that file and everything else will not be considered and not lead to failing the helm validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants