Changed the structure of the cron condition for ISM policy CRD

rkthtrifork commented 3 months ago

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

[x] 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
[x] No linter warnings (make lint)

If CRDs are changed:

[x] CRD YAMLs updated (make manifests) and also copied into the helm chart
[x] 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 commented 3 months ago

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

swoehrl-mw commented 3 months ago

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

@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

opensearch-project / opensearch-k8s-operator