🌱 Remove kaniko / extension-developer-e2e cleanup #2116
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| export E2E_TEST_CATALOG_V1 := e2e/test-catalog:v1 | ||
| export E2E_TEST_CATALOG_V2 := e2e/test-catalog:v2 | ||
| export CATALOG_IMG := $(LOCAL_REGISTRY_HOST)/$(E2E_TEST_CATALOG_V1) | ||
| export CLUSTER_REGISTRY_HOST := $(E2E_REGISTRY_NAME).$(E2E_REGISTRY_NAMESPACE).svc:5000 |
There was a problem hiding this comment.
The naming of CLUSTER vs LOCAL for registry host was backwards IMO, so I switched it around.
There was a problem hiding this comment.
The original naming was cluster-centric... I do wonder if there are better names... e.g.
INSIDE_CLUSTER ?
OUTSIDE_CLUSTER ?
| @@ -32,6 +32,9 @@ func TestExtensionDeveloper(t *testing.T) { | |||
| require.NoError(t, corev1.AddToScheme(scheme)) | |||
| require.NoError(t, rbacv1.AddToScheme(scheme)) | |||
|
|
|||
| require.NotEmpty(t, os.Getenv("CATALOG_IMG"), "environment variable CATALOG_IMG must be set") | |||
There was a problem hiding this comment.
These fields are required to be set for the test, and if not it will fail in an unclear way.
dtfranz
force-pushed
the
remove-kaniko
branch
2 times, most recently
from
601fb2c to
5657f3d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2116 +/- ##
==========================================
+ Coverage 72.61% 73.65% +1.03%
==========================================
Files 78 78
Lines 7260 7260
==========================================
+ Hits 5272 5347 +75
+ Misses 1633 1564 -69
+ Partials 355 349 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dtfranz
force-pushed
the
remove-kaniko
branch
3 times, most recently
from
96f8952 to
843a8f1
Compare
|
Having issues with podman vs docker environments 😑 |
openshift-ci
bot
added
the
do-not-merge/hold
Removes use of kaniko, which is now an archived project, in favor of using tools we already have at our disposal. Also attempted to clean up the Makefile and setup script a bit, as well as a fail-early check in the test to make it clearer that some env is required. Signed-off-by: Daniel Franz <dfranz@redhat.com>
|
Should be fixed now |
openshift-ci
bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Nice 🚀
I loved it when we were able to delete dependencies with third parties!!!!
Just a nit and a question. Otherwise, I am OK with.
I think @tmshort should give a look as well since I think he has been working relate to it.
| .PHONY: test-ext-dev-e2e | ||
| test-ext-dev-e2e: $(OPERATOR_SDK) $(KUSTOMIZE) $(KIND) #HELP Run extension create, upgrade and delete tests. | ||
| test/extension-developer-e2e/setup.sh $(OPERATOR_SDK) $(CONTAINER_RUNTIME) $(KUSTOMIZE) $(KIND) $(KIND_CLUSTER_NAME) $(E2E_REGISTRY_NAMESPACE) | ||
| test-ext-dev-e2e: $(OPERATOR_SDK) $(KUSTOMIZE) #HELP Run extension create, upgrade and delete tests. |
There was a problem hiding this comment.
Should we not keep the $(KIND) here: test-ext-dev-e2e: $(OPERATOR_SDK) $(KUSTOMIZE)
We removed it from the shell test/extension-developer-e2e/setup.sh but I understand that the above will ensure that we have it installed.
There was a problem hiding this comment.
IMO we should limit dependency declaration like that to targets that actually use the dependency. In my mind this would be a little bit like adding $(KIND) to the e2e target; it's not necessary because e2e can't be run without the prerequisites from test-e2e already being called, and if they weren't then having $(KIND) declared as a dependency for it wouldn't help at all.
|
|
||
| kubectl wait --for=condition=Complete -n "${namespace}" jobs/kaniko-${reg_pkg_name} --timeout=60s | ||
| ${container_tool} build -f "${TMP_ROOT}/catalog.Dockerfile" -t "${catalog_push_tag}" "${TMP_ROOT}/" | ||
| ${container_tool} push ${catalog_push_tag} ${tls_flag} |
There was a problem hiding this comment.
Just to ensure that I understand it, do we need to push the catalog image for a registry build locally, because in OCP we cannot do a kind load image, am I right?
There was a problem hiding this comment.
Using a registry vs kind load is theoretically faster since kind load will transfer all image layers each time it's run, while the registry can reuse them. It's still an option, but I think the registry is probably more feature-rich.
|
/lgtm |
|
Looks reasonable |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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 |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 5dfcc76
into
operator-framework:main
Removes use of kaniko, which is now an archived project, in favor of using tools we already have at our disposal. Also attempted to clean up the Makefile and setup script a bit, as well as a fail-early check in the test to make it clearer that some env is required.
The combination of env + args for this test is still not great but hopefully it's at least a bit easier to understand.
Description
Reviewer Checklist