CI improvements

[moukoublen]

Summary of your changes

• Move opa ci to new ci workflow Update: reverted to move to separate PR.
• Upload cloudbeat logs on fail or debug
• Delete docker images from artifact only on success run (but put retention-days to 2)

Screenshot/Data

Related Issues

Checklist

• I have added tests that prove my fix is effective or that my feature works
• I have added the necessary README/documentation (if appropriate)

Introducing a new rule?

• Generate rule metadata using this script
• Add relevant unit tests
• Generate relevant rule templates using this script, and open a PR in elastic/packages/cloud_security_posture

[mergify]

This pull request does not have a backport label. Could you fix it @moukoublen? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v./d./d./d is the label to automatically backport to the 8./d branch. /d is the digit
  NOTE: backport-skip has been added to this pull request.

[github-actions]

📊 Allure Report - 💚 No failures were reported.

Result	Count
🟥 Failed	0
🟩 Passed	359
⬜ Skipped	33

[oren-zohar]

Why change workflow name?

Suggested change

	name: 'OPA CI'
	Test OPA Policies

[moukoublen]

This is mainly because it's not just testing; it involves fmt check, bundle building, test, opa check, and lint with Regal. So, I thought CI better describes the steps.

But I could change it to Test OPA Policies.

[oren-zohar]

No that's what I thought, sounds good. I just want to avoid more workflows in our actions logs, which are already pretty packed, and to avoid confusion, but I agree with you

[gurevichdmitry]

@moukoublen I'm not sure if we need to store debug logs for more than a day. Enabling debug mode in the workflow and generating logs is usually a proactive action, and typically, you'll use those logs on the same day.

[moukoublen]

Changed it to 1.

[gurevichdmitry]

Several consecutive commits will cancel the workflow run, and in such cases, we don't need to upload test results. So, we can use !cancelled() instead of always().

[moukoublen]

It used to be like that (success() || failure() == !cancelled() ), and I specifically changed it because when we have missing results, then pytest retries, and it gets eventually canceled, so the job is marked as canceled.

In this case, we want the logs/results. That's why I changed it.

[gurevichdmitry]

I'm not entirely clear on the scenario. Are you referring to situations where pytest is running and the results are incomplete when the job gets canceled? If the job is interrupted or canceled, I'm not convinced we need to retain the incomplete results.

[moukoublen]

Lets say that because of some bug in code or testcase, or cloud resource configuration, we end up with some expected results to be missing.

Pytest retries trying to find the expected rule with the expected status.

In that case, the job's timeout might be reached before the pytest ends with an exit code that indicates an error.

If this happens, we must upload the test results to identify which test case didn't pass.

I don't remember the specific CI that this happened I could search.

Perhaps we could better sync pytest timeout vs job timeout?

[gurevichdmitry]

Not necessary to search for the specific run, I understand now. In this case, using always() will address the issue, but it might introduce noise when the job is canceled due to a new commit being submitted.
Okay, let's try using always() and see if it behaves as expected for our needs.

[moukoublen]

Yes, unfortunately, it introduces a bit of noise and a small delay on consecutive commits. 😞

Let's see if it becomes a bummer we can revert back to not canceled.

[gurevichdmitry]

The same applies here; I believe using !cancelled() is better than always()

[gurevichdmitry]

Added some comments