🔥 Migrating to Makefile Modules ALL AT ONCE 🔥 #556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maelvls
changed the title
|
I will continue this on Monday, Aug 19th if that's OK. I really enjoy working with Makefile Modules. |
maelvls
commented
Aug 19, 2024
Comment on lines
-11
to
+12
| fmt.Println("Preflight version: ", version.PreflightVersion, version.Platform) | ||
| fmt.Println("Preflight version: ", version.PreflightVersion, runtime.GOOS+"/"+runtime.GOARCH) |
There was a problem hiding this comment.
Self-review: I've (somewhat) checked that the old version.Platform had the same values as the new runtime.GOOS+"/"+runtime.GOARCH since it used to be set to
-X "github.com/jetstack/preflight/pkg/version.Platform=$(GOOS)/$(GOARCH)" \
which were set using
GOOS:=$(shell go env GOOS)
GOARCH:=$(shell go env GOARCH)
which matches the new Go string above.
maelvls
force-pushed
the
step1-makefile-modules
branch
3 times, most recently
from
fdf4cac to
ab0f53f
Compare
maelvls
added a commit
that referenced
this pull request
Aug 19, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
maelvls
added a commit
that referenced
this pull request
Aug 20, 2024
Note that I should probably have gone with a fake of the ConnectionHandler instead of an envtest. We will move to the fake later on. I added the venaficonnection CRDs manually for now. I have a PR to automate pulling these CRDs from the venafi-connection-lib project: #556 For now, I added these manifests manually with the following commands: gh pr checkout 556 git checkout - git checkout step1-makefile-modules -- deploy/charts/venafi-kubernetes-agent/templates/venafi-connection-crd{,.without-validations}.yaml
3 tasks
inteon
reviewed
Sep 11, 2024
| repo_ref: main | ||
| repo_hash: 2547c81aaa2ff4aeefdda53988191b5cbe929985 | ||
| repo_path: modules/oci-publish | ||
| - folder_name: repository-base |
There was a problem hiding this comment.
I think we should probably skip this module for now. It is very specific for cert-manager repos atm. (eg. creates the OWNERS_ALIASES file).
inteon
reviewed
Sep 11, 2024
| // pull the CRD manifest from the venafi-connection-lib project. | ||
| func main() { | ||
| fmt.Print(string(crd.VenafiConnectionCrd)) | ||
| } |
There was a problem hiding this comment.
@maelvls you can take a look at https://github.com/jetstack/venafi-enhanced-issuer which has some logic for generating and modifying these Venafi Connection CRDs.
|
Thanks for the review, Tim. For context, I paused working on this because I have been focusing on VC-35568 which (I was told) is higher priority. I want to resume working on this as soon as I make some progress on VC-35568. Although I'd love to continue working on this, I think it can wait one week or two. WDYT? |
maelvls
force-pushed
the
step1-makefile-modules
branch
from
ab0f53f to
2373a00
Compare
Initially, my goal was to just renamed the Makefile to make/02_mod.mk, and change nothing else, with the intent of migrating bit by bit. After a few attempts, I found that the fact that the Makefile is being run within a container makes things needlessly complex, and trying to make makefile-modules work in that context isn't worth it. That's why I propose to migrate everything at once, with the goal of making no breaking changes to the Helm charts and containers (except for the binary location, binary name, entrypoint, and cmd).
maelvls
force-pushed
the
step1-makefile-modules
branch
from
2373a00 to
cce1879
Compare
maelvls
commented
Sep 26, 2024
| @@ -1,23 +1,20 @@ | |||
| # THIS FILE IS AUTOMATICALLY GENERATED. DO NOT EDIT. | |||
| # Edit https://github.com/cert-manager/makefile-modules/blob/main/modules/repository-base/base/.github/dependabot.yaml instead. | |||
There was a problem hiding this comment.
Broken link, I've fixed this upstream: cert-manager/makefile-modules#196
maelvls
force-pushed
the
step1-makefile-modules
branch
from
6b3b365 to
3aa2fe8
Compare
Using set -x, I noticed that LDFLAGS= was being split on the commas: + LDFLAGS='-X version.Commit=6b3b365dbf4a3a907b6fd97d745efbd889afde0f -X version.BuildDate=Thu,' + 26 Sep 2024 14:34:51 '+0000 -X client.ClientID=k3TrDbfLhCgnpAbOiiT2kIE1AbovKzjo To fix this, I've decided to print the date without spaces or special chars. Before, it looked like this: Fri, 27 Sep 2024 10:15:10 +0000 Now, it looks like this: 2024-09-27-12:15:16-CEST
maelvls
force-pushed
the
step1-makefile-modules
branch
from
3aa2fe8 to
3c98fb5
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
These markers are only meant for properties that don't have a default value.
These markers are only meant for properties that don't have a default value.
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
inteon
force-pushed
the
step1-makefile-modules
branch
from
775fa69 to
83a24f1
Compare
inteon
reviewed
Sep 30, 2024
…pending Co-authored-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
maelvls
force-pushed
the
step1-makefile-modules
branch
from
a16d1d3 to
f20e5fb
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
@maelvls I think the PR is ready to be merged.
|
|
Thanks for working on the GitHub Actions. I've looked at them and they look OK. Let's merge this and try releasing v1.1.0-alpha.0 to see if everything works. |
inteon
approved these changes
Sep 30, 2024
The official registry is oci://eu.gcr.io/jetstack-secure-enterprise/charts, but we haven't configured access to this GCR registry in GitHub Actions for now, so we will be pushing to Quay for now.
Merged
wallrj
reviewed
Oct 10, 2024
| # -- Specify image pull credentials if using a private registry | ||
| # example: - name: my-pull-secret | ||
| # Overrides the image tag whose default is the chart appVersion. | ||
| tag: "v0.0.0" |
There was a problem hiding this comment.
@maelvls This default value makes it into the README file which may confuse users:
$ helm inspect readme _bin/scratch/image/venafi-kubernetes-agent-1.1.0-dirty.tgz | fgrep -A 5 image.tag
#### **image.tag** ~ `string`
> Default value:
> ```yaml
> v0.0.0
> ```make helm-chart uses yq to modify this value after running helm package.
There was a problem hiding this comment.
The image.tag value is not on the documentation website:
There was a problem hiding this comment.
Well spotted, that’s a problem.
- I’ll let Michael know about the outdated Helm reference page,
- I propose that we remove the default tag and only show an example
I can open a PR with thisI created a ticket: https://venafi.atlassian.net/browse/VC-36700
There was a problem hiding this comment.
I propose that we remove the default tag and only show an example; I can open a PR with this.
👍
Sounds like a good idea, I think that will work.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've revived #555. Here is why I don't think we should do gradually:
make build-docker-imageswas running a make command within a container in a buildx env, which made things super complicated for nothingCGO_ENABLED=1set, I would understand. But since we don't, we should Ko.This PR is my ongoing effort to fully migrate this repo to makefile-modules.
A few things are already working:
make helm-chart make oci-push-preflight # To generate jetstack.io_venaficonnections.yaml: make generate-manifests🚨 MAYBE BREAKING 🚨 The image's binary location isn't the same. The images are vastly different...
/bin/preflightand is now located at/ko-app/preflight.Full diff:
Notes:
quay.io/jetstack/preflight, and to havequay.io/jetstack/venafi-agentas the only image we push since that's the image that gets mirrored. The old Helm chartdeploy/charts/jetstack-securecan be updated to usequay.io/jetstack/venafi-agentif needed.Tests Performed