-
Notifications
You must be signed in to change notification settings - Fork 255
HIVE-1939: Add local e2e test support #2818
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
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
@huangmingxia: This pull request references HIVE-1939 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: huangmingxia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Add Makefile target test-e2e-local and refactor local-e2e-test.sh to auto-detect configuration and provide sensible defaults.
ca0c0f7 to
8bf5e8d
Compare
2uasimojo
left a comment
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.
I got partway through this before realizing it's a Draft and you probably don't want me looking at it yet :)
...But it was a useful exercise because I think I realized what I want the user experience to be. Rather than a whole separate script that sets up all the env vars and runs e2e in a subprocess, what if the UX was more like:
$ source hack/e2e_env_setup.sh
$ make test-e2e # or `make test-e2e-pool`Lots of the cool interactive discovery you've done here can be reused. But then the other advantage is that I can run e2e twice without having to redo all of that.
As part of this work, e2e-common.sh could start with an env check to make sure all the vars it expects are set.
| # Local e2e test script - easier to run e2e tests locally | ||
| # This script provides sensible defaults and can be customized via environment variables | ||
| # | ||
| # Usage: |
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 is nice, but can we make it the output of the command if help is requested? Normally I would say that would be the case if an argument like -?, -h, --help, or help was provided; but in this case it seems like this script normally accepts no arguments, so we could just print it if any argument is given.
| if command -v oc &> /dev/null; then | ||
| DETECTED_RELEASE_IMAGE=$(KUBECONFIG="$KUBECONFIG" oc get clusterversion version -o=jsonpath='{.status.desired.image}' 2>/dev/null || true) | ||
| elif command -v kubectl &> /dev/null; then | ||
| DETECTED_RELEASE_IMAGE=$(KUBECONFIG="$KUBECONFIG" kubectl get clusterversion version -o=jsonpath='{.status.desired.image}' 2>/dev/null || true) | ||
| fi |
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.
We're going to need oc in the actual test suite no matter what, so we may as well check for it once, at the top level of this library (or at least via a func that's called from local-e2e-test.sh), and fail if it's not found.
(It may be possible to run e2e with kubectl standing in for oc -- I haven't tried. If that's the case, then the suggested func could be a little cleverer and, in the absence of oc but the presence of kubectl, run something like alias oc=kubectl.)
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.
Thank you! If oc is unavailable in the local environment, kubectl can also be used for some queries. However, the ideal situation is still to use oc.
| fi | ||
| else | ||
| # Auto-detect: use default kubeconfig location if KUBECONFIG is not set | ||
| DEFAULT_KUBECONFIG="${HOME}/.kube/config" |
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.
I appreciate that we're trying to make this as seamless as possible, but since this test is not guaranteed to be nondestructive, I don't think we should try so hard to discover a default.
In fact, I'm not super convinced we should even use whatever $KUBECONFIG is currently set in the env -- we should consider requiring it to be provided explicitly, either via a specially-named env var ($HIVE_E2E_HUB_KUBECONFIG?) or a CLI argument (--hub-kubeconfig). If they want to use their $KUBECONFIG they can always do so (--hub-kubeconfig $KUBECONFIG).
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.
Yes, I can pass the kubeconfig by adding the --hub-kubeconfig parameter. I actually added this option at first, but then removed it later. The reason I removed it was that I was planning for maximum automation, similar to how it's done in CI, pass the cloud parameter to automatically trigger the subsequent e2e tests.
However, after thinking about it again, I now see your point. It makes sense to include these parameters, as well as --help. Thanks a lot!
|
|
||
| # Check cluster connectivity using oc or kubectl | ||
| # Requires KUBECONFIG to be set (should be called after setup_kubeconfig) | ||
| function check_cluster_connectivity() { |
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.
Nit: cluster is ambiguous; suggest using hub or spoke, as appropriate, throughout.
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.
Thanks! Here should verify whether the provided hub cluster is available.
| # Setup HIVE_IMAGE: auto-detect, prompt, or use existing value | ||
| # Must export the variable so e2e-common.sh and child processes can access it | ||
| function setup_hive_image() { | ||
| if [ -z "$HIVE_IMAGE" ]; then |
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.
Here's where I think we could default to $IMG since that's what the rest of the Makefile uses.
(While I'm here, I noticed that Makefile var defaults to something that's almost certainly obsolete for everyone. Can we get rid of that?)
(Later) I see that e2e-common.sh is doing this the other way around... but I think it would still work to chicken-egg-chicken the var from here, if set.
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.
Got it - I will check this.
| function setup_hive_image() { | ||
| if [ -z "$HIVE_IMAGE" ]; then | ||
| echo "HIVE_IMAGE not set, attempting to get latest tag from quay.io..." | ||
| if ! command -v curl &> /dev/null || ! command -v jq &> /dev/null; then |
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.
Again, jq is required by e2e-test.sh, so let's add it to the "things to check for up front" function mentioned elsewhere.
| exit 1 | ||
| fi | ||
| else | ||
| HIVE_IMAGE_REPO="${HIVE_IMAGE_REPO:-redhat-user-workloads/crt-redhat-acm-tenant/hive-operator/hive}" |
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 is kind of cool -- if I set $HIVE_IMAGE_REPO to point to my personal one, it'll still go out and try to discover the latest image...
...but only if I built an index, which I normally don't. I can't think of a great way to discover that the most recent image in my personal repo is a legit hive container image... but again, we may be trying too hard here. Perfect is the enemy of done :)
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.
in my personal repo is a legit hive container image.
Thank you -
I'm not sure if the information retrieved by this command can be used for verification, Entrypoint is /opt/services/manager.
I'll go learn how to verify whether the provided Hive image is a legit Hive container image.
podman inspect quay.io/redhat-user-workloads/crt-redhat-acm-tenant/hive-operator/hive:hive-on-push-ml47k-build-image-index | jq '.[0].Config.Entrypoint'
[
"/opt/services/manager"
]
% podman run --rm quay.io/skopeo/stable:latest inspect --override-arch amd64 --override-os linux docker://quay.io/redhat-user-workloads/crt-redhat-acm-tenant/hive-operator/hive:hive-on-push-ml47k-build-image-index'
...
"Labels": {
"architecture": "x86_64",
"build-date": "2025-12-18T23:04:10Z",
"com.redhat.component": "hive-rhel9",
"com.redhat.license_terms": "https://www.redhat.com/en/about/red-hat-end-user-license-agreements#UBI",
"cpe": "cpe:/a:redhat:enterprise_linux:9::appstream",
"description": "Hive is an operator which runs as a service on top of Kubernetes/OpenShift. The Hive service can be used to provision and perform initial configuration of OpenShift clusters",
"distribution-scope": "public",
"io.buildah.version": "1.41.4",
"io.k8s.description": "Hive is an operator which runs as a service on top of Kubernetes/OpenShift. The Hive service can be used to provision and perform initial configuration of OpenShift clusters",
"io.k8s.display-name": "hive-operator",
"io.openshift.expose-services": "",
"io.openshift.tags": "cluster,management,provision",
"maintainer": "Red Hat, Inc.",
"name": "hive",
"org.opencontainers.image.created": "2025-12-18T23:04:10Z",
"org.opencontainers.image.revision": "b061ef0a98a594fa2099647cf228495566317c5f",
"release": "1",
"summary": "API driven OpenShift 4 cluster provisioning and management",
"url": "https://github.com/openshift/hive",
"vcs-ref": "b061ef0a98a594fa2099647cf228495566317c5f",
"vcs-type": "git",
"vendor": "Red Hat, Inc.",
"version": "1"
},
"Architecture": "amd64",
"Os": "linux",
"Layers": [
"sha256:46a9484471e55e0e501c08ff903616330af0505ba749ef70e8c87e103e07844a",
"sha256:b8daaea900e82c4617ef1a3b0a24c2a1597570ec225d03bf40feeff2173dddbb"
],
"LayersData": [
{
"MIMEType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
"Digest": "sha256:46a9484471e55e0e501c08ff903616330af0505ba749ef70e8c87e103e07844a",
"Size": 40040759,
"Annotations": null
},
{
"MIMEType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
"Digest": "sha256:b8daaea900e82c4617ef1a3b0a24c2a1597570ec225d03bf40feeff2173dddbb",
"Size": 563249031,
"Annotations": null
}
],
"Env": [
"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
"container=oci",
"SMDEV_CONTAINER_OFF=1",
"HOME=/home/hive"
]
}
| fi | ||
| fi | ||
| fi | ||
| export HIVE_IMAGE="$HIVE_IMAGE" |
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.
nit: I believe this can just be spelled
| export HIVE_IMAGE="$HIVE_IMAGE" | |
| export HIVE_IMAGE |
| fi | ||
| fi | ||
| fi | ||
| fi |
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 may be more of a stylistic preference, but generally when I see this many layers of indent it's a signal that the code could be simplified to reduce branching and improve readability/maintainability. Applies throughout. (Did an AI help you write this?)
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.
Yes - I provided the AI with the command to extract the image, and it generated the corresponding function :)
| echo "Error: HIVE_IMAGE cannot be empty" >&2 | ||
| exit 1 | ||
| fi | ||
| else |
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.
If we're accepting an input or using a default, do we want to go check that the thing exists in quay, and that we have access to it?
(Note: If I had been writing this, I probably wouldn't have gone to such great lengths to cover every possible case. But since you've got a path that queries the quay API anyway...)
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.
Thank you, that makes a lot of sense - I didn't think to check whether we have access to this image.
I found the validate_image function logic from bundle-gen.py, maybe we can use similar logic, and I'll check it again.
|
@2uasimojo I really appreciate your help with the review!! It's been incredibly helpful. I will update this draft according to your comments and add the e2e-pool test configuration. |
Add Makefile target test-e2e-local and refactor local-e2e-test.sh to use common setup functions with interactive prompts and auto-detection for easier local testing.