Skip to content

QoL: add early checks for being logged in to quay.io #1207

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.

Sign up for GitHub

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

Closed
metlos wants to merge 2 commits into from
Closed

QoL: add early checks for being logged in to quay.io #1207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccbadcd
add early checks for being logged in to quay.io to not waste time on …
metlos Sep 25, 2025
78b47b1
Merge branch 'master' into check-quay-login
rsoaresd Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions make/dev.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ dev-deploy-e2e-local: deploy-e2e-local-to-dev-namespaces print-reg-service-link
dev-deploy-e2e-local-two-members: deploy-e2e-local-to-dev-namespaces-two-members print-reg-service-link

.PHONY: deploy-e2e-local-to-dev-namespaces
deploy-e2e-local-to-dev-namespaces:
deploy-e2e-local-to-dev-namespaces: check-quay-login-if-needed
$(MAKE) deploy-e2e-local MEMBER_NS=${DEFAULT_MEMBER_NS} SECOND_MEMBER_MODE=false HOST_NS=${DEFAULT_HOST_NS} REGISTRATION_SERVICE_NS=${DEFAULT_HOST_NS} ENVIRONMENT=${DEV_ENVIRONMENT} E2E_TEST_EXECUTION=false
$(MAKE) setup-dev-sso DEV_SSO=${DEV_SSO}

.PHONY: deploy-e2e-local-to-dev-namespaces-two-members
deploy-e2e-local-to-dev-namespaces-two-members:
deploy-e2e-local-to-dev-namespaces-two-members: check-quay-login-if-needed
$(MAKE) deploy-e2e-local MEMBER_NS=${DEFAULT_MEMBER_NS} MEMBER_NS_2=${DEFAULT_MEMBER_NS_2} HOST_NS=${DEFAULT_HOST_NS} REGISTRATION_SERVICE_NS=${DEFAULT_HOST_NS} ENVIRONMENT=${DEV_ENVIRONMENT} E2E_TEST_EXECUTION=false
$(MAKE) setup-dev-sso DEV_SSO=${DEV_SSO}

Expand Down
22 changes: 22 additions & 0 deletions make/podman.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# WILL_PUBLISH is true when the e2e goals will publish new operator image(s)
# to quay. This enables the check to conditionally execute only when needed.
WILL_PUBLISH := false
ifneq ($(HOST_REPO_PATH),)
WILL_PUBLISH := true
endif
ifneq ($(MEMBER_REPO_PATH),)
WILL_PUBLISH := true
endif
ifneq ($(REG_REPO_PATH),)
WILL_PUBLISH := true
endif

.PHONY: check-quay-login-if-needed
check-quay-login-if-needed:
@if [ "${WILL_PUBLISH}" = "true" -o "${PUSH_SANDBOX_IMAGE}" = "true" ]; then \
podman login --get-login quay.io >/dev/null; \
fi

.PHONY: check-quay-login
check-quay-login:
@podman login --get-login quay.io >/dev/null
Comment on lines +13 to +22
Copy link
Copy Markdown
Collaborator

@MatousJobanek MatousJobanek Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be easier to execute the check everytime when any makefile target is executed and the WILL_PUBLISH is true?

at first, we would need to set the variable aslo when PUSH_SANDBOX_IMAGE is true:

ifeq ($(PUSH_SANDBOX_IMAGE),true)
WILL_PUBLISH := true
endif

and then either use .EXTRA_PREREQS which is in available in GNU Make 4.3+ (not sure how this would work with Mac

.PHONY: check-quay-login
check-quay-login:
ifeq ($(WILL_PUBLISH),true)
	@podman login --get-login quay.io >/dev/null
endif

.EXTRA_PREREQS := check-quay-login

or second option do it via .SHELLSTATUS, which should work everywhere

ifeq ($(WILL_PUBLISH),true)
_ := $(shell podman login --get-login quay.io >/dev/null)
ifneq ($(.SHELLSTATUS),0)
$(error ERROR: Not logged in to quay.io.)
endif
endif

Copy link
Copy Markdown
Collaborator

@MatousJobanek MatousJobanek Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found another option - this could also work:

ifeq ($(WILL_PUBLISH),true)
_ := $(or $(shell podman login --get-login quay.io >/dev/null && echo ok),$(error ERROR: Not logged in to quay.io))
endif

4 changes: 2 additions & 2 deletions make/sandbox-ui.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ deploy-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator --output
deploy-sandbox-ui: REGISTRATION_SERVICE_API=https://$(shell oc get route registration-service -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')/api/v1
deploy-sandbox-ui: HOST_OPERATOR_API=https://$(shell oc get route api -n ${HOST_NS} -o custom-columns=":spec.host" | tr -d '\n')
deploy-sandbox-ui: RHDH=https://rhdh-${SANDBOX_UI_NS}.$(shell oc get ingress.config.openshift.io/cluster -o jsonpath='{.spec.domain}')
deploy-sandbox-ui:
deploy-sandbox-ui: check-quay-login-if-needed
Copy link
Copy Markdown
Contributor

@mfrancisc mfrancisc Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add it to push-sandbox-plugin so that it covers both deploy-sandbox-ui and test-sandbox-ui-in-container. And eventually future targets that will have to push the image.

Copy link
Copy Markdown
Contributor Author

@metlos metlos Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to perform the check as early as possible. So we really need to find the 'entry points' into the different 'flows' that we invoke using make and figure out a proper place where to add this check.

This is my stab at the entry point to the flow of deploying the UI. @rsoaresd, could you please take a look and tell what good places this check should be put?

Copy link
Copy Markdown
Contributor

@rsoaresd rsoaresd Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, going to take a look! Just saw the PR and the comment, sorry for the delay

Copy link
Copy Markdown
Contributor

@rsoaresd rsoaresd Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metlos, if the goal is to check as early as possible, I suggest adding at the top level of deploy-sandbox-ui and test-sandbox-ui-in-container make target (which I saw you already did).

For now, push-sandbox-plugin is used on:

  • .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml, where we already log in to quay before calling push-sandbox-plugin, so we are good
  • when running make test-sandbox-ui-in-container
  • when running test-ui-e2e or test-ui-e2e-local locally which calls deploy-sandbox-ui

@mfrancisc has a good point too, if we add future make targets, we could miss adding the early check for being logged into quay.io

$(MAKE) check-sso-credentials
@echo "sandbox ui will be deployed in '${SANDBOX_UI_NS}' namespace"
$(MAKE) create-namespace SANDBOX_UI_NS=${SANDBOX_UI_NS}
Expand Down Expand Up @@ -216,7 +216,7 @@ build-sandbox-ui-e2e-tests:

# Run Developer Sandbox UI e2e tests image using podman
.PHONY: test-sandbox-ui-in-container
test-sandbox-ui-in-container: build-sandbox-ui-e2e-tests
test-sandbox-ui-in-container: check-quay-login build-sandbox-ui-e2e-tests
@echo "pushing Developer Sandbox UI image..."
$(MAKE) push-sandbox-plugin
@echo "running the e2e tests in podman container..."
Expand Down
6 changes: 3 additions & 3 deletions make/test.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ prepare-and-deploy-e2e: prepare-e2e deploy-e2e
@echo "To clean the cluster run 'make clean-e2e-resources'"

.PHONY: verify-migration-and-deploy-e2e
verify-migration-and-deploy-e2e: prepare-projects e2e-deploy-latest e2e-migration-setup get-publish-and-install-operators e2e-migration-verify
verify-migration-and-deploy-e2e: check-quay-login-if-needed prepare-projects e2e-deploy-latest e2e-migration-setup get-publish-and-install-operators e2e-migration-verify
Copy link
Copy Markdown
Contributor

@mfrancisc mfrancisc Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you already added the check as part of get-publish-and-install-operators , so not sure if we need to add it again here as well.

Copy link
Copy Markdown
Contributor Author

@metlos metlos Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the whole point of this PR. To do the login check earlier during the build, particularly prior to deploying the latest from the repo (the e2e-deploy-latest target) which is very time consuming. I don't want to spend 10 mins running the tests only to realize my quay.io session has expired.

The reason to put this check also into get-publish-and-install-operators is because there are different 'flows' in the Makefiles some of which don't go through the verify-migration step, yet still would end up pushing to quay.

I'm not sure I actually cover all the 'entry points' in the best possible way but I hope what is in this PR is at least a good starting point.

Copy link
Copy Markdown
Collaborator

@MatousJobanek MatousJobanek Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, do you still face the issue after using the generated token?


.PHONY: e2e-migration-setup
e2e-migration-setup:
Expand Down Expand Up @@ -307,7 +307,7 @@ get-publish-install-and-register-operators: get-and-publish-host-operator get-an
get-publish-and-install-operators: get-and-publish-host-operator create-host-resources get-and-publish-member-operator

.PHONY: get-and-publish-member-operator
get-and-publish-member-operator: ksctl
get-and-publish-member-operator: check-quay-login-if-needed ksctl
ifneq (${MEMBER_NS_2},"")
ifneq (${MEMBER_NS_2},)
$(eval MEMBER_NS_2_PARAM = -mn2 ${MEMBER_NS_2})
Expand Down Expand Up @@ -336,7 +336,7 @@ else
endif

.PHONY: get-and-publish-host-operator
get-and-publish-host-operator: ksctl
get-and-publish-host-operator: check-quay-login-if-needed ksctl
ifneq (${REG_REPO_PATH},"")
ifneq (${REG_REPO_PATH},)
$(eval REG_REPO_PATH_PARAM = -rr ${REG_REPO_PATH})
Expand Down
Loading