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

feat: add masked field to role #762

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

cgroschupp
Copy link
Contributor

@cgroschupp cgroschupp commented Mar 20, 2024

Description

Add masked fields to OpensearchRole

Close #765

@prudhvigodithi
Copy link
Collaborator

Hey @cgroschupp thanks for your contribution, before we get this PR merged, can you please open an issue summarizing this change? This way your contribution is tracked in the issue and later we can link this PR to the issue.

Adding @bbarani @salyh @jochenkressin @pchmielnik @swoehrl-mw

@cgroschupp
Copy link
Contributor Author

@prudhvigodithi done

@salyh
Copy link
Collaborator

salyh commented Mar 28, 2024

Code changes LGTM but CI fails (https://github.com/opensearch-project/opensearch-k8s-operator/actions/runs/8359361554/job/23179427774?pr=762)

@prudhvigodithi not sure if the CI failure is related to #767

Copy link
Collaborator

@salyh salyh left a comment

Choose a reason for hiding this comment

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

Make CI green

@cgroschupp
Copy link
Contributor Author

@salyh i found the issue with envtest and golang 1.19. i changed envtest to release-16

Signed-off-by: Christian Groschupp <christian@groschupp.org>
@cgroschupp
Copy link
Contributor Author

@salyh forget my last comment, i rebased by branch to use golang 1.22

@salyh
Copy link
Collaborator

salyh commented Mar 30, 2024

@cgroschupp Before we can get this PR merged, please refer to the PR guideline and add the mssing prerequisites. Thank you very much.

@prudhvigodithi
Copy link
Collaborator

LGTM, @swoehrl-mw or @salyh WDYT?

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.

  • New field needs to be documented in docs/designs/crd.md
  • The extended CRD needs to be copied into the operator helm chart

@cgroschupp
Copy link
Contributor Author

cgroschupp commented Apr 2, 2024

@swoehrl-mw docs/designs/crd.md is very outdated. Maybe it is better to generate it, see commit: c16cc73

Signed-off-by: Christian Groschupp <christian@groschupp.org>
@swoehrl-mw
Copy link
Collaborator

docs/designs/crd.md is very outdated. Maybe it is better to generate it, see commit: c16cc73

@cgroschupp
A good idea, but this should be properly introduced in a separate PR that makes sure all parts of the old reference are in code comments and removes the old file. And ideally it should introduce a new automated PR check that the docs have been generated.

Signed-off-by: Christian Groschupp <christian@groschupp.org>
@cgroschupp
Copy link
Contributor Author

@swoehrl-mw i have updated the tests and copied the crds to the helm chart directory. for the api docs generation task i will create a new pr.

@swoehrl-mw
Copy link
Collaborator

@salyh Can you please also approve and then merge (seems merge is only allowed after all reviewers who have requested changes have approved)?

@salyh salyh self-requested a review April 4, 2024 14:33
@salyh salyh merged commit 41ce0c7 into opensearch-project:main Apr 4, 2024
9 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.

[FEATURE] Add maskedFields to OpensearchRole
4 participants