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

Make Makefiles reusable and automate release process #195

Merged
merged 3 commits into from
May 13, 2024

Conversation

inteon
Copy link
Member

@inteon inteon commented Oct 10, 2023

ref: cert-manager/makefile-modules#3

Note

The trust-bundle adds some extra complexity

  1. The trust-bundle image uses a file in the "/debian-package" folder as copy-source, adding this file is not possible through ko and has to be done in an extra step. Luckily, we can reuse the layer-append logic that we have for the licenses module.
  2. trust-bundles are versioned separately from trust-manager. However, when we do a trust-manager release, the Helm chart must refer to an existing trust-bundle. For that reason, the trust-manager release process starts with a oci-maybe-push-package_debian action to make sure the referenced trust package exists.
  3. To simplify the release process, both trust-manager and the debian trust-bundle are published using GH actions now. This reduces the number of different systems (removes gcb) we have & pipeline secrets we have. It does however add extra changes to this PR.

ℹ️ I tested these changes on my personal branch and fixed all remaining issues in the release process.

@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Oct 10, 2023
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from inteon. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 10, 2023
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023
@jetstack-bot jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2023
@jetstack-bot jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 20, 2023
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2023
@inteon inteon force-pushed the cicd branch 3 times, most recently from 4956df7 to bd6e51e Compare October 31, 2023 09:47
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@inteon inteon force-pushed the cicd branch 6 times, most recently from 6f8fe69 to 5e22cf6 Compare October 31, 2023 18:39
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2023
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2023
@inteon inteon force-pushed the cicd branch 3 times, most recently from 34d928c to 6906bde Compare November 14, 2023 19:42
@cert-manager-prow cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2024
@inteon inteon force-pushed the cicd branch 9 times, most recently from 874f9a0 to eedc3df Compare May 8, 2024 12:28
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

One thing I spotted!

I think it'd be nice and not too difficult to roll this change out in a way that doesn't require breaking the tests at any point, but I guess we can always roll the tests back so I don't think that's a requirement.

EDIT: That said, since the tests are failing right now maybe that would be a good idea so we can have some confidence before we merge

make/debian-trust-package.mk Outdated Show resolved Hide resolved
@inteon
Copy link
Member Author

inteon commented May 10, 2024

@SgtCoDFish I re-added the old make targets for backwards compatibility, now we don't have to break the tests.

Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

I think this is good to go and kudos to @inteon for the effort it took to get this done.

I'm adding a hold because Tim isn't working today as far as I can tell (despite the updates he made this morning!). This can wait until Monday to merge, and then Tim should be available to help with the followup PRs to cleanup the testing repo / remove the deprecated make targets afterwards.

@cert-manager-prow cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 10, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2024
@inteon
Copy link
Member Author

inteon commented May 13, 2024

/unhold

@cert-manager-prow cert-manager-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2024
@inteon inteon merged commit a7798d5 into cert-manager:main May 13, 2024
4 checks passed
fi

echo "{}" | jq \
--rawfile bundle /etc/ssl/certs/ca-certificates.crt \
Copy link
Member

Choose a reason for hiding this comment

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

note: IMPORTANT

I missed this during the review. I shouldn't have missed it and it'll need to fixed.

This uses the system's trust-bundle, not the one from the container.

It breaks development on all platforms which aren't Debian-based linux (e.g. macOS, RHEL, etc)

It shouldn't lead to an incorrect bundle being pushed upstream, but it needs fixing as a matter of urgency in case that changes in the future.

See #355

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing & fixing @SgtCoDFish!
I made a typo indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants