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

Add extraManifests value #27

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Conversation

CiucurDaniel
Copy link
Contributor

This PR implements #26

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

thanks for offering the help! some last polishings and we're good!

charts/zipkin/ci/extraManifests-values.yaml Show resolved Hide resolved
charts/zipkin/values.yaml Show resolved Hide resolved
@CiucurDaniel
Copy link
Contributor Author

Done with the requested changes

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

thanks!

@codefromthecrypt codefromthecrypt merged commit fe57c92 into openzipkin:master Nov 19, 2024
@@ -143,3 +143,12 @@ extraServicePorts: []

# global variables to use in values. typically used in tests.
global: {}

# extra manifests to deploy with the helm chart
extraManifests: []
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be extraManifests: {} to match the object type?

Copy link
Member

Choose a reason for hiding this comment

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

PR welcome and merged quickly to fix any mistakes! One way even more ideal is to add ci tests as we have for some other options, but I'll leave that up to the person to decide to do or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcuz no, array is correct. If you want to be more accurate you can change it to be array of objects [ {} ].

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the fix in #32 seems to work. 😄

Copy link
Member

Choose a reason for hiding this comment

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

thanks @marcuz and @CiucurDaniel. If either of you have time to add a fake value in a ci file, please do, so that the mystery stays solved https://github.com/openzipkin/zipkin-helm/tree/master/charts/zipkin/ci

that or add the linting not currently happening to our workflow config
https://github.com/openzipkin/zipkin-helm/blob/master/.github/workflows/test.yml#L72

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

Successfully merging this pull request may close these issues.

3 participants