search

opensearch-project / opensearch-k8s-operator

OpenSearch Kubernetes Operator
Apache License 2.0
366 stars 192 forks source link

feat: add masked field to role #762

Closed cgroschupp closed 3 months ago

cgroschupp commented 3 months ago

Description

Add masked fields to OpensearchRole

Close #765

prudhvigodithi commented 3 months ago

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 commented 3 months ago

@prudhvigodithi done

salyh commented 3 months ago

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 https://github.com/opensearch-project/opensearch-k8s-operator/pull/767

cgroschupp commented 3 months ago

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

cgroschupp commented 3 months ago

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

salyh commented 3 months ago

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

prudhvigodithi commented 3 months ago

LGTM, @swoehrl-mw or @salyh WDYT?

cgroschupp commented 3 months ago

@swoehrl-mw docs/designs/crd.md is very outdated. Maybe it is better to generate it, see commit: https://github.com/opensearch-project/opensearch-k8s-operator/pull/762/commits/c16cc732d254060a6cca5e81333a2274099a0d4a

swoehrl-mw commented 3 months ago

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.

cgroschupp commented 3 months ago

@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 commented 3 months ago

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