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

Fixing values.yaml docs for schema generation, corrected values.schema.json #474

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

julian-perge
Copy link
Contributor

@julian-perge julian-perge commented Oct 4, 2024

With 3.9.0, the values.schema.json is created incorrectly from the values.yaml, therefore not allowing any extra env values from manager.env to be placed in there since it's requiring an object, but env is supposed to be an array of objects.

Should fix #471

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2024

CLA assistant check
All committers have signed the CLA.

@julian-perge julian-perge force-pushed the jp/fixing-values-schema-mapping branch 2 times, most recently from 0c76c98 to a794f8b Compare October 4, 2024 17:29
@yorugac
Copy link
Collaborator

yorugac commented Oct 6, 2024

Hi @julian-perge, thanks for the PR.
According to the #471, there is probably a whole list of values that should be changed in the schema, if not all of them. If you'd like to work on that issue, #471, you're welcome to, but the schema overall should be fixed, not just a part of it.
Also, just in case, signing CLA is mandatory for this repo 🙂

@julian-perge julian-perge force-pushed the jp/fixing-values-schema-mapping branch 3 times, most recently from 7203bd9 to 271f44e Compare October 7, 2024 23:03
@julian-perge
Copy link
Contributor Author

julian-perge commented Oct 7, 2024

Hi @julian-perge, thanks for the PR. According to the #471, there is probably a whole list of values that should be changed in the schema, if not all of them. If you'd like to work on that issue, #471, you're welcome to, but the schema overall should be fixed, not just a part of it. Also, just in case, signing CLA is mandatory for this repo 🙂

Thanks for the issue link, and I signed the CLA, but had my signing keys messed up. Should be fixed now.

Will probably work on improving the schema throughout the rest of the week when in down time.

@julian-perge julian-perge force-pushed the jp/fixing-values-schema-mapping branch from 271f44e to 16739bc Compare October 7, 2024 23:40
@julian-perge julian-perge changed the title Fixing manager.env schema and added manager.envFrom Fixing values.yaml docs for schema generation, corrected values.schema.json Oct 7, 2024
@julian-perge
Copy link
Contributor Author

@yorugac Updated PR to include the rest of the schema fixes.

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

Hi @julian-perge, apologies for the delay! I've finally tested this: it seems that tolerations should also be changed to array - please see my comment below.
That's the only request from me: the rest seems to be working, AFAIS 🙌 Would you be able to update that part?

# required: false
# type: object
# @schema
# affinity -- Affinity to be applied on all containers
affinity: {}

# @schema
# additionalProperties: true
# required: false
# type: object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# type: object
# type: array

I'm getting the following error now:

- tolerations: Invalid type. Expected: object, given: array

And tolerations are normally defined as an array:
https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/

Copy link
Collaborator

Choose a reason for hiding this comment

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

It appears this change request was done in a new PR by another contributor over here:
https://github.com/grafana/k6-operator/pull/481/files

Copy link
Collaborator

@yorugac yorugac left a comment

Choose a reason for hiding this comment

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

@julian-perge, I'll be merging your PR as is now, and merge the fix for tolerations from #481. I hope that's alright with you!

Thank you again, for fixing this 🙌

@yorugac yorugac merged commit d54a2e1 into grafana:main Oct 31, 2024
8 checks passed
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.

When specifying values in the Helm chart, the installation fails.
3 participants