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

002 design document grafana crd v1beta1 #684

Merged
merged 12 commits into from
Mar 8, 2022

Conversation

NissesSenap
Copy link
Collaborator

@NissesSenap NissesSenap commented Feb 13, 2022

Description

A design document how the new v1beta1 grafana crd should look like.

The more feedback that we can get from the community on this the better.

Relevant issues/tickets

#667 roadmap 5.0

Type of change

  • Design document
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

Verification steps

Edvin Norling added 9 commits February 13, 2022 15:31
Still have some TODO and need to verify how it works in OCP.
Need to implement test to see how it would actually work.
I'm leaning towards alternative1 but it's easier to talk about smaller changes in the document.
At the same time I will have to rewrite the document to actually reflect the proposal
Easier to just do in a sperate PR towards the tmp v5 repo
## Proposal

In short the proposal is about moving the grafana container specific config from the main GrafanaSpec to GrafanaContainer.
GrafanaContainer should contain [V1.Container](https://pkg.go.dev/k8s.io/api@v0.20.2/core/v1?utm_source=gopls#Container) but with removal of a number of required values like name that will be hard coded to `grafana`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like that idea. But how can we remove fields from the ContainerSpec that we don't want to expose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was unclear, we need to use something like the operator-tools for the deploymentOverrides but for ContainerSpec.
I don't think it exist in the current package. For starters we can add it to our code and where we only supply the fields that we think should be made available. From the top of my head, the only one we should remove is name since it always should be grafana.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can replicate the function logic for deploymentOverride to implement some really simple blocklists, and just reject a CR definition if it tries to override a disallowed field

@HVBE
Copy link
Collaborator

HVBE commented Mar 4, 2022

@NissesSenap I think we can merge this? Unless, youd still liike to extend it a bit?

@NissesSenap
Copy link
Collaborator Author

It works for me. Let's merge it and if someone have a opinion they can create a PR and we can discuss it

@AdheipSingh
Copy link
Contributor

@NissesSenap just a question, are you planning a conversion webhook ?

@NissesSenap
Copy link
Collaborator Author

Probably not, 5.0 will be a complete rewrite of the operator and we don't want to merge legacy code. How important do you think it is @AdheipSingh ?

@NissesSenap NissesSenap merged commit f632af3 into grafana:master Mar 8, 2022
@NissesSenap NissesSenap deleted the grafana_api_002 branch May 30, 2022 06:02
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.

None yet

4 participants