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

Changed the structure of the cron condition for ISM policy CRD #838

Merged

Conversation

rkthtrifork
Copy link
Contributor

@rkthtrifork rkthtrifork commented Jun 4, 2024

Description

The cron condition for ISM policy CRDs was not working since it had the wrong structure. The structure in the operator was defined as

...
"conditions": {
  "cron": {
    "expression": "* 17 * * SAT",
    "timezone": "America/Los_Angeles"
  }
}
...

while the OpenSearch API expects

...
"conditions": {
  "cron": {
    "cron": {
      "expression": "* 17 * * SAT",
      "timezone": "America/Los_Angeles"
    }
  }
}
...

This PR should resolve the issue.

I also ran into a few odd details in the code where we checked if an error was nil when it would always be nil so I have moved those lines out of the if-statement. A few error messages were capital first letter which was different from the rest of the codebase so I fixed those. Lastly, my linter (default Go extension for VSCode) swapped some import orders. Let me know if those changes are alright.

Issues Resolved

I don't think this PR solves any current issues

Check List

  • Commits are signed per the DCO using --signoff
  • Unittest added for the new/changed functionality and all unit tests are successful
  • Customer-visible features documented
  • No linter warnings (make lint)

If CRDs are changed:

  • CRD YAMLs updated (make manifests) and also copied into the helm chart
  • Changes to CRDs documented - (there was no documentation for this part of the CRD, but I am happy to make one if you would like)

Please refer to the PR guidelines before submitting this pull request.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@rkthtrifork rkthtrifork marked this pull request as ready for review June 6, 2024 07:38
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.

Hi @rkthtrifork. Thanks for contributing this change.

Just to clarify: Are you saying that in its old state the cron condition just did not work? I'm asking because technically this change of the CRD would be considered breaking as it changes existing fields and would require a major release. But if it never worked in the first place we can forgoe it as nobody will have used it.

The code looks good, but please make sure the CRD yaml is also copied into the operator helm chart.

I also ran into a few odd details in the code [...] Let me know if those changes are alright.

These are all fine.

@rkthtrifork
Copy link
Contributor Author

Hi @rkthtrifork. Thanks for contributing this change.

Just to clarify: Are you saying that in its old state the cron condition just did not work? I'm asking because technically this change of the CRD would be considered breaking as it changes existing fields and would require a major release. But if it never worked in the first place we can forgoe it as nobody will have used it.

The code looks good, but please make sure the CRD yaml is also copied into the operator helm chart.

I also ran into a few odd details in the code [...] Let me know if those changes are alright.

These are all fine.

Yes, the cron condition never worked. I'll get the CRD yaml into the helm chart

@rkthtrifork rkthtrifork requested a review from swoehrl-mw June 7, 2024 07:52
@swoehrl-mw
Copy link
Collaborator

@rkthtrifork. Looks good. Can you fix the DCO for your commits (https://github.com/opensearch-project/opensearch-k8s-operator/pull/838/checks?check_run_id=25930444133)? Then I can approve and merge.

@rkthtrifork
Copy link
Contributor Author

rkthtrifork commented Jun 7, 2024

@rkthtrifork. Looks good. Can you fix the DCO for your commits (https://github.com/opensearch-project/opensearch-k8s-operator/pull/838/checks?check_run_id=25930444133)? Then I can approve and merge.

I'm not sure why its complaining about sign-off, both commits seem to me to be both signed off and verified, but I'll rebase the branch and see if that fixes the issue

@rkthtrifork rkthtrifork force-pushed the patch/fix-cron-condition branch 2 times, most recently from 9747656 to 836eb0d Compare June 7, 2024 08:38
@rkthtrifork rkthtrifork force-pushed the patch/fix-cron-condition branch from 836eb0d to 46466ba Compare June 7, 2024 08:40
@swoehrl-mw swoehrl-mw merged commit 418af82 into opensearch-project:main Jun 7, 2024
10 checks passed
@rkthtrifork rkthtrifork deleted the patch/fix-cron-condition branch June 7, 2024 11:56
@prudhvigodithi prudhvigodithi mentioned this pull request Jun 19, 2024
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.

2 participants