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

Switch to makefile modules #49

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

inteon
Copy link
Member

@inteon inteon commented Sep 10, 2024

Start using the makefile-modules solution to test, build and release the sample-external-issuer.

@cert-manager-prow
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 assign wallrj for approval. 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

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 10, 2024
@inteon inteon force-pushed the switch_to_makefile_modules branch 2 times, most recently from ee7011d to 448efed Compare September 10, 2024 08:45
@inteon inteon force-pushed the switch_to_makefile_modules branch 2 times, most recently from 5a45949 to ed4c3f8 Compare September 19, 2024 12:31
@inteon inteon force-pushed the switch_to_makefile_modules branch from ed4c3f8 to 6f3a224 Compare November 25, 2024 14:44
Signed-off-by: Tim Ramlot <[email protected]>
@inteon inteon force-pushed the switch_to_makefile_modules branch from 6f3a224 to 7069d1b Compare November 25, 2024 15:21
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.

I haven't reviewed every line of this (obviously!) but it looks like what I'd expect.

That said, while makefile-modules makes sense here for ease of maintenance, this is quite a different repo to our others in that we expect others to copy+paste this repo, effectively.

If we do that, we're encouraging issuer maintainers to use makefile-modules. Today mm is basically an implementation detail of how we maintain the cert-manager projects, but this change would encourage others to use it.

Have we as a group discussed that or thought about the implications of that? I'm not saying we definitely shouldn't do that, but it does feel worth a thought

OWNERS_ALIASES Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

comment: Honestly kind of a shame to remove this - it would be a great easy way for external issuer maintainers to package up their issuer, no?

@erikgb
Copy link

erikgb commented Dec 18, 2024

If we do that, we're encouraging issuer maintainers to use makefile-modules. Today mm is basically an implementation detail of how we maintain the cert-manager projects, but this change would encourage others to use it.

Encouraging our users to use makefile-modules will force us to think a lot more around breaking changes. Do we really want this? I would prefer to keep no guarantees on compatibility for makefile-modules and just use it for our the projects that are maintained by us (the cert-manager team).

I think this project should be built around kubebuilder (as it is, more or less) and kept up to date. I saw this PR when considering preparing a PR for migrating away from the use of the long-term deprecated kube-rbac-proxy. Kubebuilder has already documented this migration, and I was planning to upgrade kubebuilder to fix this now urgent issue. See also https://kubernetes.slack.com/archives/CDEQJ0Q8M/p1734534667142689. But I don't want to do this with this PR undecided.

@wallrj
Copy link
Member

wallrj commented Jan 3, 2025

I agree with @SgtCoDFish and @erikgb .
I want freedom to update and refactor makefile-modules without having to worry about breaking external issuers that have bootstrapped from this repo.
So I vote to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. 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.

4 participants