-
Notifications
You must be signed in to change notification settings - Fork 67
🌱 Test Registry #1425
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
🌱 Test Registry #1425
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
fa691f6 to
2355c52
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1425 +/- ##
==========================================
+ Coverage 73.42% 73.45% +0.03%
==========================================
Files 42 42
Lines 3063 3063
==========================================
+ Hits 2249 2250 +1
+ Misses 641 640 -1
Partials 173 173
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
verdiff is complaining about new |
|
I have a wicked simple bundle that can be installed: https://github.com/tmshort/test-image |
If you're suggesting this to replace the I did also just test that we can definitely get away with just having the test-catalog's bundles all point to the same |
|
It's a bundle, it can be installed... Not sure how critical it is to actually have something with a CRD or deployment. That being said, it's just an offer. Not making it a requirement. |
f005a89 to
7b7b8e0
Compare
| ./hack/test/build-push-e2e-catalog.sh $(E2E_REGISTRY_NAMESPACE) $(LOCAL_REGISTRY_HOST)/$(E2E_TEST_CATALOG_V2) | ||
| E2E_REGISTRY_IMAGE=localhost/e2e-test-registry:devel | ||
| image-registry: ## Build the testdata catalog used for e2e tests and push it to the image registry | ||
| go build $(GO_BUILD_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/registry/bin/registry ./testdata/registry/registry.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| go build $(GO_BUILD_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/registry/bin/registry ./testdata/registry/registry.go | |
| GOOS=linux GOARCH=amd64 go build $(GO_BUILD_FLAGS) -tags '$(GO_BUILD_TAGS)' -ldflags '$(GO_BUILD_LDFLAGS)' -gcflags '$(GO_BUILD_GCFLAGS)' -asmflags '$(GO_BUILD_ASMFLAGS)' -o ./testdata/registry/bin/registry ./testdata/registry/registry.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the test-e2e target has a dependency on the image-registry target, the GOOS and GOARCH variables should be set from the test-e2e target, and don't need to be set explicitly here. This way, you can build image-registry locally, and get the correct (local) images when no running the e2e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to use GOOS=$(go env GOOS) GOARCH=$(go env GOARCH) instead?
This approach would make the target compatible with both macOS and Linux environments, allowing us to run it locally as well. If we explicitly set GOARCH=amd64, for example, it won’t run on macOS systems with a different architecture, where the expected result would be darwin/arm64. Is targeting linux/amd64 specifically what we're aiming for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. current changes take this into account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that's the right approach. Doing the export should work when doing the image-target, and setting the variable equal to the go env ought to be a noop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tmshort,
Not sure that's the right approach. Doing the export should work when doing the image-target, and setting the variable equal to the go env ought to be a noop.
I’m not sure if I fully follow your comment here. The export has fixed values as well:
image-registry: export GOOS=linux
image-registry: export GOARCH=amd64
Either way, this suggests fixed values for go build.
So, how would GOOS=linux GOARCH=amd64 go build build a binary for darwin/arm64 (my local env, for example)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but doing this:
GOOS=$(go env GOOS)
Is basically setting the GOOS variable to the environment value that the go command already has available to it. It's equivalent to doing:
GOOS=${GOOS}
Which is a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 I didn't answer your question.
So, how would
GOOS=linux GOARCH=amd64 go buildbuild a binary for darwin/arm64 (my local env, for example)?
It wouldn't. In this particular case, it's building executables for the docker container, which have to be linux/amd64. These two executables are never expected to run locally.
My point is that doing GOOS=$(go env GODS) is pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This build is specifically for the image-registry image to be run in kind; comparing to how we build the operator-controller's manager binary, we use the same GOOS=linux GOARCH=amd64 ENV when building (make run uses docker-build which then uses build-linux). I'm just following that same convention.
If you feel it's important, what we could potentially do is split up the image-registry target so that GOOS=linux GOARCH=amd64 is only set when actually building the image. But I think this is unnecessary or at least non-critical enough that we could address it later in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two executables are never expected to run locally.
If that is the case, (we never run it locally and would not make sense run it locally) then I am OK with fixed values 👍
b0f71b4 to
1ec1910
Compare
e5b06f3 to
8ef3a4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Adds a bare-bones test registry and a folder tree for our test images. Signed-off-by: dtfranz <[email protected]>
8ef3a4c to
2f2d220
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
After the squash, it's still the same.
66ecaac
Test Registry
Adds a bare-bones test registry and a folder tree for our test images.
This PR aims to improve our test infrastructure by removing scripts and external images for registry operations in favor of bare-bones tools built in golang using well-known libraries (
go-containerregistryin particular).Structure
The PR adds two tools:
registryandpush. It also aggregates all our image-related makefile targets into one:image-registry.registryJust about the simplest possible image registry that can be built using the
go-containerregistrylib. See README here.pushUtilizes
cranewithingo-containerregistryto build and push our catalog and bundle images. It uses a directory walker which is intended to make adding future test bundles and catalogs easily. It also (IMO - totally subjective) makes our test image collection more WYSIWYG by clearly showing all the images and tags we use in the folder rather than hiding them in scripts or makefiles.See README here for more info on the expected folder structure.
make image-registryBuilds the
registryandpushbinaries, bundles them into one image, then deploys them both.registryis deployed essentially as before.pushexecutes as a job, building all the images bundled inside of it then pushing them to the specified registry address.TODOs / Follow-Ups
PR size is currently huge due to the amount of prometheus manifests being copied for each bundle tag... I'm open to suggestions on how we could address this. Maybe we could remove theAddressed this, see below.prometheus-operatorin favor of a test operator with fewer manifests?Remaining items to address:
Either:Done - going with option C for now.Find a better way to organize tags such that we need not have many copies of the same manifests.Keep the current folder structure for tags but simplify the manifests we're using in test to reduce file count/complexity.prometheus-operatoruses the same bundle image (v1.0.0). We were previously doing this anyway essentially because each tag we created for this bundle had the exact same content.Metadata reading for bundle images is using one path instead of reading dynamically (which we can get away with right now because all the bundles are identical).Done.Configurable cert/key path forDone.registry(not super critical right now).Use mounted secrets forDone.pushinstead ofcrane.Insecure(also not critical).Description
Reviewer Checklist