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

ISM Policies support #575

Merged

Conversation

IshaGirdhar
Copy link
Contributor

@IshaGirdhar IshaGirdhar commented Jul 28, 2023

This PR fixes #435
Add structs to generate the ISM policy CRDs
Add roles
Add a controller to reconcile the ISM policy
Add Reconciler to create/update and delete the ISM policy from the OS cluster.
Add helper functions.
Add documentation

@swoehrl-mw
Copy link
Collaborator

Hi @IshaGirdhar. How does this new PR relate to your old #555? Does it supersede it so the old one can be closed?

@IshaGirdhar
Copy link
Contributor Author

Hi @IshaGirdhar. How does this new PR relate to your old #555? Does it supersede it so the old one can be closed?

yes old one can be closed.

@IshaGirdhar IshaGirdhar marked this pull request as ready for review August 7, 2023 09:48
Copy link
Contributor

@AndersBennedsgaard AndersBennedsgaard left a comment

Choose a reason for hiding this comment

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

Just a couple of small remarks. I'm not a Go expert, but I've skimmed through the code and it looks mainly good to me

go.work Outdated Show resolved Hide resolved
docs/userguide/main.md Outdated Show resolved Hide resolved
@IshaGirdhar
Copy link
Contributor Author

@idanl21 @swoehrl-mw Please review.

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Please have a look at my inline comments.
Also plase add tests for the new functionality (you can use users_test.go or role_test.go as a blueprint).

docs/userguide/main.md Outdated Show resolved Hide resolved
opensearch-operator/api/v1/opensearchism_types.go Outdated Show resolved Hide resolved
opensearch-operator/pkg/reconcilers/ismpolicy.go Outdated Show resolved Hide resolved
opensearch-operator/pkg/reconcilers/ismpolicy.go Outdated Show resolved Hide resolved
@IshaGirdhar
Copy link
Contributor Author

Please have a look at my inline comments. Also plase add tests for the new functionality (you can use users_test.go or role_test.go as a blueprint).

Done

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

  • Also add the yamls for the new CRD to the PR (both in config/crd/bases and in the helm chart)
  • Make sure you are dealing with situations where the policyId is not set (fall back to metadata.name)
  • You need to detect situations where policyId is changed and error out (the field should basically be treated as immutable)
  • See inline comments

opensearch-operator/pkg/reconcilers/ismpolicy.go Outdated Show resolved Hide resolved
opensearch-operator/pkg/reconcilers/ismpolicy.go Outdated Show resolved Hide resolved
docs/userguide/main.md Outdated Show resolved Hide resolved
docs/userguide/main.md Outdated Show resolved Hide resolved
docs/userguide/main.md Show resolved Hide resolved
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>

Fix test

Fix test
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
@AndersBennedsgaard
Copy link
Contributor

I've noticed that the https://github.com/Opster/opensearch-k8s-operator/blob/main/opensearch-operator/PROJECT file hasn't been kept up-to-date lately. Additionally, the newly introduced CRD hasn't been added to it. This is because developers haven't utilized a command like kubebuilder create api --group opensearch.opster.io --version v1 --kind ISMPolicy, which would automatically update the PROJECT file.

Considering that we aren't currently using it for any specific purpose, such as OLM integration, should we consider removing it altogether (this can be done in another PR)? Alternatively, should we make a concerted effort to keep it updated?

Reference to the PROJECT file can be found here: https://book.kubebuilder.io/reference/project-config.html

@swoehrl-mw
Copy link
Collaborator

I've noticed that the https://github.com/Opster/opensearch-k8s-operator/blob/main/opensearch-operator/PROJECT file hasn't been kept up-to-date lately. Additionally, the newly introduced CRD hasn't been added to it. This is because developers haven't utilized a command like kubebuilder create api --group opensearch.opster.io --version v1 --kind ISMPolicy, which would automatically update the PROJECT file.

Considering that we aren't currently using it for any specific purpose, such as OLM integration, should we consider removing it altogether (this can be done in another PR)? Alternatively, should we make a concerted effort to keep it updated?

Reference to the PROJECT file can be found here: https://book.kubebuilder.io/reference/project-config.html

Good point, it doesn't hurt to have the file, so I'd say keep it and try to keep it consistent.

Signed-off-by: Isha Girdhar <[email protected]>
Signed-off-by: Isha Girdhar <[email protected]>
@AndersBennedsgaard
Copy link
Contributor

@IshaGirdhar would you mind adding the CRD to the PROJECT file?
@swoehrl-mw when this is done, could you approve this PR, or are we missing something?

@swoehrl-mw
Copy link
Collaborator

@AndersBennedsgaard

when this is done, could you approve this PR, or are we missing something?

My last review comments were all addressed, so should be good to go.

Signed-off-by: Isha Girdhar <[email protected]>
@swoehrl-mw swoehrl-mw merged commit 2a69528 into opensearch-project:main Nov 10, 2023
6 checks passed
FatmaBouzghaia pushed a commit to FatmaBouzghaia/opensearch-k8s-operator that referenced this pull request Dec 5, 2023
This PR fixes opensearch-project#435 
Add structs to generate the ISM policy CRDs
Add roles 
Add a controller to reconcile the ISM policy 
Add Reconciler to create/update and delete the ISM policy from the OS
cluster.
Add helper functions.
Add documentation
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.

ISM Policies Support
3 participants