QoL: add early checks for being logged in to quay.io#1207
Conversation
…e2e tests that are going to fail because of that
WalkthroughAdds conditional and unconditional quay.io login checks and a WILL_PUBLISH flag; introduces podman-based login targets; and wires the new login checks into several Makefile targets including deploy, test, and operator publish workflows (with DEPLOY_LATEST handling). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Make as Make Target
participant Login as check-quay-login-if-needed
participant Podman as podman
participant Flow as Deploy/Publish Steps
Dev->>Make: invoke target (e.g., deploy-e2e-local-to-dev-namespaces)
Make->>Login: prerequisite
alt WILL_PUBLISH==true OR PUSH_SANDBOX_IMAGE==true
Login->>Podman: podman login quay.io
Podman-->>Login: success / failure
else
Login-->>Make: skipped (no-op)
end
Make->>Flow: proceed with target steps
Flow-->>Dev: completion
sequenceDiagram
autonumber
actor Dev as Developer
participant Make as test-sandbox-ui-in-container
participant Login as check-quay-login
participant Podman as podman
participant Build as build-sandbox-ui-e2e-tests
participant Test as Run E2E in container
Dev->>Make: invoke
Make->>Login: prerequisite (always)
Login->>Podman: podman login quay.io
Podman-->>Login: result
Make->>Build: build image/tests
Make->>Test: execute tests
Test-->>Dev: results
sequenceDiagram
autonumber
actor Dev as Developer
participant Make as get-and-publish-*-operator
participant Params as Param Eval
participant Latest as Install Latest (DEPLOY_LATEST)
participant Script as Publish Script (with FORCED_TAG/REPO_PATHs)
Dev->>Make: invoke
Make->>Params: compute MEMBER_NS_2 / HOST/REG/MEMBER_REPO_PATH / FORCED_TAG
alt DEPLOY_LATEST==true
Make->>Latest: install latest operator(s)
else
Make->>Script: run publish with computed params
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
make/podman.mk (1)
14-18: Consider adding error handling for login validation.The conditional login check uses
--get-loginwhich returns an error if no login is found. While the/dev/nullredirection suppresses output, consider whether the target should fail when authentication is required but not present, or if it should provide a more informative error message..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 + @if [ "${WILL_PUBLISH}" = "true" -o "${PUSH_SANDBOX_IMAGE}" = "true" ]; then \ + if ! podman login --get-login quay.io >/dev/null 2>&1; then \ + echo "Error: Not logged in to quay.io. Please run 'podman login quay.io' first." >&2; \ + exit 1; \ + fi; \ + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
make/dev.mk(1 hunks)make/podman.mk(1 hunks)make/sandbox-ui.mk(2 hunks)make/test.mk(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: GolangCI Lint
- GitHub Check: Unit Tests
- GitHub Check: Build & push Developer Sandbox UI image for UI e2e tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (8)
make/dev.mk (1)
46-46: LGTM! Proper integration of authentication checks.The addition of
check-quay-login-if-neededprerequisites to both local deployment targets ensures authentication is validated before proceeding with potentially time-consuming operations. This aligns well with the PR's goal of avoiding wasted time on deployments that would fail due to authentication issues.Also applies to: 51-51
make/sandbox-ui.mk (2)
38-38: LGTM! Authentication check properly integrated.Adding
check-quay-login-if-neededas a prerequisite todeploy-sandbox-uiensures quay.io authentication is validated before starting the deployment process. This follows the consistent pattern established across other deployment targets in the PR.
214-214: Unconditional quay login is appropriate here
This target always invokespush-sandbox-plugin, so requiringcheck-quay-loginunconditionally aligns with its behavior.make/podman.mk (2)
1-12: LGTM! Clean and logical flag derivation.The
WILL_PUBLISHlogic correctly identifies when operator images need to be published by checking for non-empty repository paths. The conditional approach avoids unnecessary authentication checks when no publishing is intended.
20-22: LGTM! Clean unconditional authentication check.The unconditional login check provides a straightforward way to verify quay.io authentication when explicitly required.
make/test.mk (3)
65-65: LGTM! Authentication check properly integrated into migration workflow.Adding
check-quay-login-if-neededto theverify-migration-and-deploy-e2etarget ensures authentication is validated before starting the migration verification process, which is crucial for avoiding wasted time in lengthy migration tests.
310-336: LGTM! Well-structured parameter evaluation and deployment logic.The
get-and-publish-member-operatortarget properly:
- Adds the authentication prerequisite
- Uses proper parameter evaluation with empty string checks
- Implements clean conditional logic for latest vs. specific version deployment
- Maintains consistency with the existing pattern
339-361: LGTM! Consistent implementation with member operator logic.The
get-and-publish-host-operatortarget follows the same well-structured pattern as the member operator target, ensuring consistency across the codebase while properly integrating authentication checks.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, metlos The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, going to take a look! Just saw the PR and the comment, sorry for the delay
There was a problem hiding this comment.
@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 callingpush-sandbox-plugin, so we are good- when running
make test-sandbox-ui-in-container - when running
test-ui-e2eortest-ui-e2e-locallocally which callsdeploy-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
|
|
||
| .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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
btw, do you still face the issue after using the generated token?
|
|
||
| .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 |
There was a problem hiding this comment.
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
endifand 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-loginor 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
endifThere was a problem hiding this comment.
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
|
|
Will not merge. This is better solved by configuring docker with quay.io token. |
5 participants
add early checks for being logged in to quay.io to not waste time on e2e tests that are going to fail anyway because of that.
Summary by CodeRabbit
New Features
Chores