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

Runtime device #46

Merged
merged 6 commits into from
Nov 21, 2024
Merged

Runtime device #46

merged 6 commits into from
Nov 21, 2024

Conversation

MaxHerbs
Copy link

Added runtimedevice and schema description

@marcelldls
Copy link
Contributor

Can I suggest we rather pass through only runtimeClassName into the spec dictionary?
The reasons being:

  • Potential for clashes with existing keys
  • Not inventing new properties in the chart (please point to existing schemas so we get the correct validation + dont have to maintain them)

My suggestion is

    spec:
      {{- if .Values.runtimeClassName }}
      runtimeClassName: {{ .Values.runtimeClassName }}
      {{- end }}

@gilesknap
Copy link
Member

gilesknap commented Nov 21, 2024

@marcelldls I asked Max to use the approach you used for hostMounts etc.

I agree that this opens up clashes as you can override the whole podSpec, but I also did not want to release a new ec-helm-charts each time we needed a new podSpec item.

In my head the model is that we provide opinionated templating for the fields that most IOCs will make use of, but leave customisation capabilities reasonably open.

I'm not strongly attached to this model and @marcelldls helm skills are much more fresh that mine so I'll go with what you vote for.

@MaxHerbs to do the schema as Marcell requests, you want to refer out to the K8S manifest schema that you are duplicating e.g. see "resources": just above your addition to the template.

@marcelldls
Copy link
Contributor

@gilesknap I agree with your model just not the full customisation being exposed in ioc-instance I believe being explicit about what we configure for most of our applications will make it easier to on board the majority that are not familiar with k8s. For full customisation, we can of-course just create own chart in the beamline repo.

@MaxHerbs
Copy link
Author

I've made the changed but haven't tested it. The beamline is in use today so follow up when I've verified it.

@gilesknap
Copy link
Member

OK, lets go with that then. @MaxHerbs please can you switch to @marcelldls' suggestion above and update the schema to reflect that change.

(I'm not sure that calling out to the K8S schema would work for a single field - but if it does I guess you will get the correct K8S description of the field and formatting restrictions for the string)

@marcelldls
Copy link
Contributor

@MaxHerbs The schema would look as follows:

        "runtimeClassName": {
          "$ref": "https://kubernetesjsonschema.dev/v1.18.1/_definitions.json#/definitions/io.k8s.api.core.v1.PodSpec/properties/runtimeClassName"
        },

@marcelldls
Copy link
Contributor

Thanks! LGTM

@gilesknap
Copy link
Member

Thanks both, merging ...

@gilesknap gilesknap merged commit 44cb1d0 into main Nov 21, 2024
3 checks passed
@gilesknap
Copy link
Member

@MaxHerbs you can do the honours and release this as 4.1.3.

Do it from this page https://github.com/epics-containers/ec-helm-charts/releases

  • Draft new Release
  • Choose Tag and type in 4.1.3
  • Generate Release Notes
  • Publish Release

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