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 org CRD in helm chart #116

Merged
merged 28 commits into from
Oct 15, 2024
Merged

add org CRD in helm chart #116

merged 28 commits into from
Oct 15, 2024

Conversation

QuantumEnigmaa
Copy link
Contributor

Towards giantswarm/roadmap#3695

This PR only adds the CRD for the organization as defined here. Follow-up PRs will be created to add the related services/controllers in the operator.

Checklist

  • Update changelog in CHANGELOG.md.
  • Follow deployment test procedure in the tests/manual_e2e directory and have a working branch.

@QuantumEnigmaa QuantumEnigmaa self-assigned this Oct 7, 2024
@QuantumEnigmaa QuantumEnigmaa requested a review from a team as a code owner October 7, 2024 15:45
@QuantumEnigmaa QuantumEnigmaa marked this pull request as draft October 7, 2024 15:53
PROJECT Outdated Show resolved Hide resolved
PROJECT Outdated Show resolved Hide resolved
@QuentinBisson
Copy link
Contributor

You're missing thé CRD into the helm chart right?

@QuantumEnigmaa QuantumEnigmaa marked this pull request as ready for review October 14, 2024 09:30
main.go Outdated Show resolved Hide resolved
Co-authored-by: Quentin Bisson <[email protected]>
api/v1alpha1/grafanaorganization_types.go Outdated Show resolved Hide resolved
api/v1alpha1/grafanaorganization_types.go Outdated Show resolved Hide resolved
api/v1alpha1/grafanaorganization_types.go Outdated Show resolved Hide resolved
api/v1alpha1/grafanaorganization_types.go Outdated Show resolved Hide resolved
api/v1alpha1/grafanaorganization_types.go Show resolved Hide resolved
api/v1alpha1/grafanaorganization_types.go Outdated Show resolved Hide resolved
kind: GrafanaOrganization
metadata:
labels:
app.kubernetes.io/name: grafanaorganization
Copy link
Member

Choose a reason for hiding this comment

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

Also what is instance supposed to contain ?
Isn't this name label redundant with the CR name ?

Suggested change
app.kubernetes.io/name: grafanaorganization
app.kubernetes.io/name: MyOrganizationName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this name label redundant with the CR name ?

You're right.

Also what is instance supposed to contain ?

I think it's supposed to be the operator, so here : olly-op

Copy link
Member

Choose a reason for hiding this comment

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

It does not makes sense to have instance, name, and managed-by labels if they all have the same value in the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree but that's what we have for all of our operators if I'm not mistaken 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Components that are part of the application should have this label to be able to tell where they come from. What would be this label used for in the context of a GrafanaOrganization CR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey what do we need to do here ?

Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the following labels ?

    app.kubernetes.io/name: grafanaorganization
    app.kubernetes.io/instance: grafanaorganization-sample
    app.kubernetes.io/part-of: observability-operator

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are generated by kube-builder, I really don't care

Copy link
Member

Choose a reason for hiding this comment

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

That's an example I assumed it was created by us and not generated

Copy link
Contributor

Choose a reason for hiding this comment

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

No, anything under the samples directory is created by kube-builder

internal/controller/grafanaorganization_controller.go Outdated Show resolved Hide resolved
internal/controller/grafanaorganization_controller.go Outdated Show resolved Hide resolved
kind: GrafanaOrganization
metadata:
labels:
app.kubernetes.io/name: grafanaorganization
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the following labels ?

    app.kubernetes.io/name: grafanaorganization
    app.kubernetes.io/instance: grafanaorganization-sample
    app.kubernetes.io/part-of: observability-operator

@QuentinBisson QuentinBisson merged commit 3b383d7 into main Oct 15, 2024
8 of 9 checks passed
@QuentinBisson QuentinBisson deleted the add-org-crd branch October 15, 2024 13:04
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.

5 participants