Migrate makefiles and CI/CD #306

inteon · 2023-11-17T16:20:45Z

Works as-is, but will be better linked with https://github.com/cert-manager/testing/pull/934/files (adds caching, go vendoring and new target names)

inteon · 2023-12-13T12:55:16Z

/retest

wallrj

Thanks Tim,

This is very cool, but I want to see more documentation in the README of this project and in the README of the upstream project explaining how it works and how to contribute changes.

I feel unsure about checking all this cloned library code in make/_shared/ into the repo.
It makes it easy to git grep to see how everything works.

I checked out the branch and tried some of the new make targets.
Please document the basic make targets for contributors in the README file.

I found the make help output difficult to read.
This is how make help looks for me:

$ make
Usage: make [TARGET [TARGET ...]] [ARGUMENT=VALUE [ARGUMENT=VALUE ...]]
Generate/ Verify:
    generate-protos $(NEEDS_PROTOC) $(NEEDS_PROTOC-GEN-GO)
        Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
Testing:
    test-smoke  $(NEEDS_GINKGO)
        Smoke end-to-end tests
    test-unit $(cert_manager_crds) $(approver_policy_crds) $(NEEDS_GINKGO) $(NEEDS_ETCD) $(NEEDS_KUBE-APISERVER) $(NEEDS_KUBECTL)
        Unit tests
[shared] Build:
$(build_targets): $(bin_dir)/bin/%: FORCE | $(NEEDS_GO) $(bin_dir)/bin
        Build manager binary.
$(oci_build_targets): oci-build-%: | $(NEEDS_KO) $(NEEDS_GO) $(NEEDS_YQ) $(bin_dir)/scratch/image
        Build the oci image.
$(oci_load_targets): oci_platforms := ""
        Load docker image.
$(oci_maybe_push_targets): oci-maybe-push-%: | $(NEEDS_CRANE)
        Push docker image if tag does not already exist.
        Expected pushed images:
        - :v1.2.3, @sha256:0000001
        - :v1.2.3.sig, :sha256-0000001.sig
$(oci_push_targets): oci-push-%: oci-build-% | $(NEEDS_CRANE) $(NEEDS_COSIGN) $(NEEDS_YQ) $(bin_dir)/scratch/image
        Push docker image.
        If the tag already exists, this target will overwrite it.
        If an identical image was already built before, we will add a new tag to it, but we will not sign it again.
        Expected pushed images:
        - :v1.2.3, @sha256:0000001
        - :v1.2.3.sig, :sha256-0000001.sig
$(run_targets): run-%: | $(NEEDS_GO)
        Run a controller from your host.
[shared] Deployment:
    install $(helm_chart_archive) | $(NEEDS_HELM)
        Install controller helm chart on the current active K8S cluster.
    template $(helm_chart_archive) | $(NEEDS_HELM)
        Template the helm chart.
    uninstall $(NEEDS_HELM)
        Uninstall controller helm chart from the current active K8S cluster.
[shared] Generate/ Verify:
    generate-api-docs $(NEEDS_GOMARKDOC)
        Generate API docs for the API types.
    generate-base
        Generate base files in the repository
    generate-crds $(NEEDS_CONTROLLER-GEN)
        Generate CRD manifests.
    generate-deepcopy $(NEEDS_CONTROLLER-GEN)
        Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations.
    generate-helm-docs $(NEEDS_HELM-DOCS)
        Generate Helm chart documentation.
    generate-klone $(NEEDS_KLONE)
        Generate klone shared Makefiles
    generate $(shared_generate_targets) $(extra_generate_targets)
        Generate all generate targets.
    verify-boilerplate $(NEEDS_BOILERSUITE)
        Verify that all files have the correct boilerplate.
    verify-generate-api-docs $(NEEDS_GOMARKDOC)
        Verify that the API docs are up to date.
    verify-helm-lint $(helm_chart_archive) | $(NEEDS_HELM)
        Verify that the Helm chart is linted.
    verify-pod-security-standards $(helm_chart_archive) | $(NEEDS_KYVERNO) $(NEEDS_KUSTOMIZE) $(NEEDS_HELM)
        Verify that the Helm chart complies with the pod security standards.
    verify $(shared_generate_targets:%=verify-%) $(extra_generate_targets:%=verify-%) $(shared_verify_targets) $(extra_verify_targets)
        Verify code and generate targets.
[shared] Helm Chart:
    helm-chart $(helm_chart_archive)
        Create a helm chart
[shared] Kind cluster:
    images-preload $(images_tar_envs)
        Preload images.
    kind-cluster-clean $(NEEDS_KIND)
        Delete the Kind cluster
    kind-cluster $(kind_kubeconfig) | $(NEEDS_KUBECTL)
        Create Kind cluster and wait for nodes to be ready
    kind-logs  $(NEEDS_KIND) $(bin_dir)/artifacts
        Get the Kind cluster
[shared] Release:
    release $(NEEDS_CRANE)
        Publish all release artifacts (image + helm chart)
[shared] Self-upgrade:
    upgrade-klone $(NEEDS_KLONE)
        Upgrade klone Makefile modules to latest version
[shared] Tools:
    tools-learn-sha
        re-download all tools and learn the sha values, useful after upgrading
    tools $(TOOLS_PATHS) $(K8S_CODEGEN_TOOLS_PATHS)
        Download and setup all tools
    vendor-go $(bin_dir)/tools/go
        By default, this Makefile uses the system's Go. You can use a "vendored"
        version of Go that will get downloaded by running this command once. To
        disable vendoring, run "make unvendor-go". When vendoring is enabled,
        you will want to set the following:

        export PATH="$PWD/$(bin_dir)/tools:$PATH"
        export GOROOT="$PWD/$(bin_dir)/tools/goroot"
    which-go $(NEEDS_GO)
        Print the version and path of go which will be used for building and
        testing in Makefile commands. Vendored go will have a path in ./bin

deploy/charts/approver-policy/values.yaml

docs/api/api.md

wallrj · 2023-12-13T16:23:09Z

.github/workflows/release.yaml

Do a test release by pushing an alpha tag linked to a commit in this branch to demonstrate that this still works.

Update the README.md file with updated release process documentation, if necessary.

https://github.com/cert-manager/approver-policy/actions/runs/7220836055
https://github.com/cert-manager/approver-policy/releases/tag/untagged-442b7c6d0c2988dd8ca6

Thanks, that looks good.
The release title isn't right

But I did a test install which worked well and the diff between the current release and this alpha release contained mostly only version label changes:

diff -u <(helm template cert-manager-approver-policy-v0.11.0.tgz) <(helm template cert-manager-approver-policy-v0.12.0-alpha.0.tgz) ... -# Source: cert-manager-approver-policy/templates/crds/policy.cert-manager.io_certificaterequestpolicies.yaml +# Source: cert-manager-approver-policy/templates/crd-policy.cert-manager.io_certificaterequestpolicies.yaml

.github/workflows/release.yaml

go.mod

go.sum

klone.json

pkg/internal/approver/validation/certificaterequest.pb.go

pkg/apis/policy/v1alpha1/zz_generated.deepcopy.go

wallrj

partial review...

.gitattributes

wallrj · 2023-12-15T12:36:11Z

.gitattributes

Are any changes need to the .gitignore file?

wallrj · 2023-12-15T12:36:40Z

.github/workflows/release.yaml

Thanks, that looks good.
The release title isn't right

But I did a test install which worked well and the diff between the current release and this alpha release contained mostly only version label changes:

diff -u <(helm template cert-manager-approver-policy-v0.11.0.tgz) <(helm template cert-manager-approver-policy-v0.12.0-alpha.0.tgz) ... -# Source: cert-manager-approver-policy/templates/crds/policy.cert-manager.io_certificaterequestpolicies.yaml +# Source: cert-manager-approver-policy/templates/crd-policy.cert-manager.io_certificaterequestpolicies.yaml

deploy/charts/approver-policy/values.yaml

.../charts/approver-policy/templates/crd-policy.cert-manager.io_certificaterequestpolicies.yaml

wallrj · 2023-12-15T13:00:25Z

.github/workflows/release.yaml

-    runs-on: ubuntu-22.04
-    needs:
-      - docker-image
+          go-version: ${{ steps.go-version.outputs.result }}


Don't you prefer to point to the go.mod file here to automatically build using the latest patch version?
I do.

The problem is that the go version is not linked to the source code. Instead, it is linked to the latest version of go at that time.
I think for reproducibility and as a way to properly keep track of versions, we should explicitly version go instead.

Turns out that setup-go doesn't necessarily pick up latest version of Go:

setup-go does not pick up recent Go version leading to create vulnerable builds actions/setup-go#434

wallrj

This is looking really nice @inteon!

I hadn't appreciated how much work had gone into it.

As far as I can see, this retains all the same tests and greatly improves the release process. Happy for you to merge this now if you prefer to make further improvements in followup PRs.

/lgtm
/hold

wallrj · 2023-12-15T14:57:06Z

hack/helm/sample-chart-values.yaml

I'd forgotten about it TBH, but I added this file in #214 as a simple way to demonstrate that #213 was fixed and ensure it stayed fixed.

I'm not too attached to it, but please create an issue to re-instate such a check to the modules library in the future.

wallrj · 2023-12-15T15:08:48Z

klone.yaml

Great! Thanks for implementing that so fast.
Looks like keeping the comments was a bit of a pain.

Maybe it'd be event nicer to call it .klone.yaml, as is customary with other tool config files....but that can be future improvement if you like.

.github/workflows/release.yaml

Makefile

README.md

klone.yaml

wallrj · 2023-12-15T17:49:20Z

make/02_mod.mk

+
+	@echo "Release complete!"
+
+# TODO: remove these deprecated targets


TODO now or later?

This is for later after we change the testing repo.

Signed-off-by: Tim Ramlot <[email protected]>

inteon · 2023-12-18T14:18:05Z

@wallrj I did remove the conditional CRDs from this PR, we will have to fix that later (when we want to use this library with trust-manager).
PTAL, all issues should be resolved now.

inteon · 2023-12-18T14:33:52Z

/retest

wallrj

Thanks!

/lgtm
/approve

jetstack-bot · 2023-12-18T14:34:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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:

~~OWNERS~~ [wallrj]

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

inteon · 2023-12-18T14:42:27Z

/unhold

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 17, 2023

inteon force-pushed the cicd branch from da0f657 to 515b85b Compare November 17, 2023 16:26

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 17, 2023

inteon force-pushed the cicd branch 14 times, most recently from 7989201 to d5e6823 Compare November 22, 2023 14:06

inteon force-pushed the cicd branch from d5e6823 to e7e36e3 Compare December 1, 2023 09:26

inteon force-pushed the cicd branch from e7e36e3 to 183a81d Compare December 12, 2023 12:13

inteon mentioned this pull request Dec 12, 2023

Migrating all cert-manager sub-projects to "Makefile modules" cert-manager/makefile-modules#3

Open

11 tasks

inteon force-pushed the cicd branch 3 times, most recently from 5f4f7a2 to db2f181 Compare December 13, 2023 12:44

inteon mentioned this pull request Dec 13, 2023

Update approver-policy test targets, optimized for the ci/cd refactor cert-manager/testing#934

Merged

inteon requested a review from wallrj December 13, 2023 14:00

inteon force-pushed the cicd branch 2 times, most recently from 1cc3d18 to 4fa5e44 Compare December 13, 2023 15:25

wallrj requested changes Dec 13, 2023

View reviewed changes

inteon force-pushed the cicd branch 4 times, most recently from 988e03e to 401ae98 Compare December 14, 2023 15:56

inteon requested a review from wallrj December 15, 2023 08:45

inteon force-pushed the cicd branch 4 times, most recently from 23e2f32 to 95502a8 Compare December 15, 2023 10:23

wallrj reviewed Dec 15, 2023

View reviewed changes

inteon force-pushed the cicd branch 2 times, most recently from 499cb54 to bc595e5 Compare December 15, 2023 14:45

wallrj approved these changes Dec 15, 2023

View reviewed changes

jetstack-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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 15, 2023

inteon force-pushed the cicd branch from bc595e5 to 2452d81 Compare December 18, 2023 14:14

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023

migrate makefiles and CI/CD

b8a0022

Signed-off-by: Tim Ramlot <[email protected]>

inteon force-pushed the cicd branch from 2452d81 to b8a0022 Compare December 18, 2023 14:16

wallrj approved these changes Dec 18, 2023

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 18, 2023

jetstack-bot merged commit e419ee1 into cert-manager:main Dec 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate makefiles and CI/CD #306

Migrate makefiles and CI/CD #306

inteon commented Nov 17, 2023 •

edited

Loading

inteon commented Dec 13, 2023

wallrj left a comment

wallrj Dec 13, 2023

inteon Dec 15, 2023

wallrj Dec 15, 2023

wallrj left a comment

wallrj Dec 15, 2023

wallrj Dec 15, 2023

wallrj Dec 15, 2023

inteon Dec 15, 2023

wallrj Dec 19, 2023

wallrj left a comment

wallrj Dec 15, 2023

wallrj Dec 15, 2023

wallrj Dec 15, 2023

wallrj Dec 15, 2023

inteon Dec 15, 2023

inteon commented Dec 18, 2023

inteon commented Dec 18, 2023

wallrj left a comment

jetstack-bot commented Dec 18, 2023

inteon commented Dec 18, 2023


		@echo "Release complete!"

		# TODO: remove these deprecated targets

Migrate makefiles and CI/CD #306

Migrate makefiles and CI/CD #306

Conversation

inteon commented Nov 17, 2023 • edited Loading

inteon commented Dec 13, 2023

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inteon commented Dec 18, 2023

inteon commented Dec 18, 2023

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented Dec 18, 2023

inteon commented Dec 18, 2023

inteon commented Nov 17, 2023 •

edited

Loading