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: Support policies in folders for reusable workflows #138

Merged
merged 14 commits into from
Dec 18, 2024

Conversation

viccuad
Copy link
Member

@viccuad viccuad commented Dec 6, 2024

Description

Fix kubewarden/kubewarden-controller#806

Recommend review by commit.

See also working monorepo repo at https://github.com/viccuad/rego-policies.

Test

For monorepo policies:

For Normal policies:

Additional Information

Tradeoff

Potential improvement

TODO:

  • Implement normal CI per policy on PRs (it's marked as a nice-to-have).
  • After approval, rebase and substitute the DROP commit, consuming my fork of kubewarden/github-actions for testing, and instead bump versions for release.

@viccuad viccuad self-assigned this Dec 6, 2024
@viccuad viccuad changed the title Monorepo feat: Support policies in folders for reusable workflows Dec 6, 2024
@viccuad viccuad marked this pull request as ready for review December 6, 2024 15:19
@viccuad viccuad requested a review from a team as a code owner December 6, 2024 15:19
@viccuad viccuad force-pushed the monorepo branch 2 times, most recently from 251cbf0 to da6b4c2 Compare December 6, 2024 15:31
required: false
default: v0.32.0
default: v0.65.0
Copy link
Contributor

Choose a reason for hiding this comment

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

wow that was an old version, maybe we should use some updatecli here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, opened #139.

runs:
using: "composite"
steps:
-
name: Login to GitHub Container Registry
- id: calculate-version
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in the monorepo action instead.
We should also use a policy version variable here to make this agnostic.
The rationale is that we want to decouple the reusable actions from whatever logic we have in the other repositories.

Copy link
Member Author

@viccuad viccuad Dec 9, 2024

Choose a reason for hiding this comment

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

We should also use a policy version variable here to make this agnostic.

It's not as simple, as the GH release needs to match the git tag that it was triggered with (and use the correct OCI_TAG), only when the trigger of this workflow was indeed from a tag (e.g: v0.1.0) instead of the "monorepo tag" (policy/v0.1.0 edited into 0.1.0).

Instead of again recalculating the version from the "monorepo tag", as we do in the monorepo workflow definition, here I just add a new, non-required policy-version input, that when present changes the behaviour.

I can take this out of the action, but that means passing that information through the reusable workflows, and potentially editing workflows of the policies we have, as that breaks backwards compatibility.

Copy link
Member Author

@viccuad viccuad Dec 9, 2024

Choose a reason for hiding this comment

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

After a talk, I think you are right and the calculate_version step can be dropped. Let me see.

Copy link
Member Author

@viccuad viccuad Dec 11, 2024

Choose a reason for hiding this comment

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

Added some comments that remove the calculate-version from the policy-release action, and expect this calculation to have been taken place in the workflows, by now making policy-release.inputs.policy-version required.
Hence, amended the workflows to always calculate-version and pass this value down the chain.

Since the workflows inputs haven't changed, this is still a backwards compatible fix WRT workflows API/ABI.

(If this is is approved, I will rebase the fixup commit and drop the "DROP" commit on the rebase as expected).

@fabriziosestito
Copy link
Contributor

@viccuad note: DCO is missing

@viccuad
Copy link
Member Author

viccuad commented Dec 11, 2024

Thanks! The DCO is missing on the fixup! commit, it will get removed with the rebase.

Copy link
Member

@jvanz jvanz left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -40,7 +40,7 @@ jobs:
run: echo "version=$(sed -n 's,^version = \"\(.*\)\",\1,p' Cargo.toml)" >> $GITHUB_OUTPUT
shell: bash
- name: Check that artifacthub-pkg.yml is up-to-date
uses: kubewarden/github-actions/check-artifacthub@v3.3.5
uses: viccuad/github-actions/check-artifacthub@main
Copy link
Member

Choose a reason for hiding this comment

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

Just remember to update that for the kubewarden org repository

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
`policy-working-dir` defaults to `.` for backwards compat.
`version` gets calculated depending if `policy-version` was set, or we
have a tag that is the version, for backwards compat.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
`policy-working-dir` defaults to `.` for backwards compat.

`version` was and continues to be set, either being a tag or coming from
a `policy-version` (the second element of a tag as `PolicyName/v0.1.0`).
This is because we need a version to be able to call
`VERSION=foo make artifacthub-pkg.yml`.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
…y-rego

`policy-working-dir` defaults to `.` for backwards compat.
If `policy-version` is set, we use it for `version`. If not, we see if
there's a valid version tag that triggered the workflow, and use that
for `version` for backwards compat.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
`policy-working-dir` defaults to `.` for backwards compat.
`policy-version` is new. If set, it's used, if not, we take the
github.ref which is expected to be a tag, for backwards compat with
non-monorepo policy releases.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
`policy-working-dir` defaults to `.` for backwards compat.

Note that, while we do `git push --force`, monorepo release jobs run
with concurrency 1.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
This is needed to run the unit tests of the monorepo
opa policies.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
It may be that it doesn't exist even, like in the monorepo case.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
This makes releases named "<tag>", e.g:
like https://github.com/kubewarden/policy-server/releases/tag/v1.19.0

Instead of releases named "Release <tag>", e.g:
https://github.com/kubewarden/verify-image-signatures/releases/tag/v0.3.0

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
By expecting as input of `policy-release` GHA, we push up the stack the
calculations of it.

policy-release `input.policy-version` will always be in the form of `0.1.0`.

Since now it's required, add it to all reusable workflows that call it.
No change on those workflows is needed, they were already calculating
their versions in their `calculate-version` steps.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@viccuad
Copy link
Member Author

viccuad commented Dec 16, 2024

Rebased with a new calculate-version step, that now correctly sets the version regardless if it was run manually by passing a policy-version, it was triggered via git, or triggered via a branch for :latest, :latest-feat-foo.

Rerun the test workflow runs, which are passing, and updated their links in the PR body.

@viccuad viccuad requested a review from jvanz December 16, 2024 12:48
Now that policy-release.input.policy-version is required, always
calculate-version.

For the left side of the &&:
policy-release will push `:latest` for main runs, and `:v0.1.0` for
tags. Hence we don't need to check if the run was triggered by a tag.

For the right side of the &&:
We may want to push an artifact irrespectively if we want it published
on artifacthub, hence we don't need to check for inputs.artifacthub.

It may happen that for a run triggered without a tag, policy-version
will be empty or a previous existent tag, but it will not get used by
neither check-artifacthub nor policy-release anyways.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@viccuad viccuad marked this pull request as draft December 17, 2024 13:39
@viccuad
Copy link
Member Author

viccuad commented Dec 17, 2024

Found a bug when running tests from PRs in calculate-version step, please don't merge.

@viccuad viccuad force-pushed the monorepo branch 3 times, most recently from ffc0a25 to 0698fff Compare December 17, 2024 13:42
@viccuad
Copy link
Member Author

viccuad commented Dec 17, 2024

Ready for merge.

The bug happened when, on PRs (not a run from branch nor a run from a tag), the calculate-version step was incorrectly going through the arm of tags. For normal repos this was fine, but for monorepos, it meant that it was taking a tag of PolicyName/v0.1.0 and generating a version=olicyName/v0.1.0 instead of version=0.1.0, which breaks make artifacthub-pkg.yml.

This is now fixed, see https://github.com/kubewarden/github-actions/compare/ffc0a258bac24630fede41f0f7c2de2f771eb9e1..0698fff8784455d9278bf36fc845a9f1989eb91e.

@viccuad viccuad marked this pull request as ready for review December 17, 2024 14:01
shell: bash
if: ${{ startsWith(github.ref, 'refs/tags/') }}
if: ${{ ! startsWith(github.ref, 'refs/heads/') }}
env:
COSIGN_EXPERIMENTAL: 1
Copy link
Member

@jvanz jvanz Dec 18, 2024

Choose a reason for hiding this comment

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

As far as I can remember, we use this env to allow keyless signing. This is not necessary anymore. Therefore, we can remove this COSIGN_EXPERIMENTAL env var.

Copy link
Member Author

@viccuad viccuad Dec 18, 2024

Choose a reason for hiding this comment

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

True! Done in c3c3a10.

Newer versions of cosign don't need this env var anymore.

Signed-off-by: Víctor Cuadrado Juan <vcuadradojuan@suse.de>
@viccuad viccuad merged commit bcfa376 into kubewarden:main Dec 18, 2024
3 checks passed
@viccuad
Copy link
Member Author

viccuad commented Dec 18, 2024

Tagged in main as v3.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Weaveworks rego policies: create a monorepo structure
3 participants