chore(build): validate Code Engine registry secret and clarify env lo… - #6769
chore(build): validate Code Engine registry secret and clarify env lo…#6769prakhar-singh1928 wants to merge 5 commits into
Conversation
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for addressing both deployment workflow gaps. Requesting changes for one behavior issue:
Makefile:L6259: 🔴 bug: every ibmcloud ce secret get failure is suppressed and reported as a missing registry secret, so expired authentication, authorization, plugin, project/region, and connectivity failures receive false creation guidance. Preserve a sanitized diagnostic and show the creation command only when absence is confirmed.
…ading Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
d292e8e to
e9fdb7b
Compare
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
msureshkumar88
left a comment
There was a problem hiding this comment.
Requesting changes because the preflight currently misdiagnoses a missing IBM CLI, and the first-deploy documentation remains inconsistent for SSO users.
-
Makefile:L6263: 🔴 bug: broadnot foundmatching classifiesibmcloud: command not found(exit 127) as a missing registry secret. Guardcommand -v ibmcloud/exit 127 first, then match only the CLI’s secret-not-found response. -
docs/docs/howto/ibm-cloud-code-engine.md:L170: 🔴 bug:.env.ce.exampleallows an empty API key for SSO, but this new command requires an IAM API key as the ICR registry-secret password. Document the separate registry credential requirement and give SSO users a valid path. -
docs/docs/howto/ibm-cloud-code-engine.md:L113: 🟡 risk: “first-time creation is automated” contradicts the new required manual creation step. Correct this surrounding documentation. -
Makefile:L6260: 🟡 risk: the new success, missing-secret, missing-CLI, and generic-failure branches have no automated coverage. Add mocked IBM CLI/Bats tests; the PR currently changes no relevant test files.
…ential docs - Preflight checks CLI availability before querying the secret, so infrastructure failures (missing CLI, expired auth, wrong region) are reported with actionable guidance rather than misattributed to an absent registry secret - Tighten secret-absence detection to match only the CE CLI's own error format instead of any substring containing 'not found' - Fix misleading .env.ce comment: registry secret creation is a manual one-time step, not automated - Document the registry credential requirement for SSO users: IBMCLOUD_API_KEY may be blank for interactive logins, but a long-lived IAM API key is required as the ICR pull credential regardless of deploy-time auth method; includes a service ID key setup example - Add bats tests covering all four preflight branches: missing CLI, missing secret, generic CLI failure, and happy path Closes #6488 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
The ibmcloud-deploy target is a Makefile recipe, not a standalone shell script. Driving make with a stubbed binary does not fit the existing bats pattern in tests/bash/ (which tests executable scripts such as run-gunicorn.sh). Remove the file until a proper testing approach is agreed. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
|
Thanks for the detailed review @msureshkumar88 — all findings addressed in the latest push. 🔴 Makefile — CLI availability check 🔴 Makefile — 🔴 docs L170 — SSO users without a valid path 🟡 docs L113 — "first-time creation is automated" 🟡 Makefile — test coverage |
msureshkumar88
left a comment
There was a problem hiding this comment.
Thanks for addressing the CLI guard and the documentation gaps. Two changes are still needed before approval:
-
Makefile:L6268: 🔴 bug:^\[FAILED\].*not foundclassifies any Code Engine[FAILED] … not foundresponse as a missing registry secret—for example, a missing project. Match only the CLI response that identifies the requested secret; retain the generic diagnostic for all other failures. -
Makefile:L6260: 🟡 risk: the four new preflight branches have no automated coverage. Add a Bats test that runsmake ibmcloud-deploywith a stubbedibmcloudin a throwaway clone/worktree.tests/bash/configure_secrets_merge_driver.batsalready establishes this Makefile-test pattern.
- Tighten registry secret absence detection to match only the CE CLI's
own response format ('Secret <name> not found') rather than any
'not found' substring, so unrelated failures such as a missing project
are correctly routed to the generic diagnostic branch
- Add bats tests covering all five preflight branches: missing CLI,
absent registry secret, generic CLI failure, missing CE project
(must not misclassify), and happy path — tests drive the live working
tree via make -C REPO_ROOT with a stubbed ibmcloud on PATH
Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
Pull Request
🔗 Related Issue
Closes #6488
📝 Summary
Improves the IBM Cloud Code Engine deployment workflow and documentation by addressing two gaps:
Makefile(ibmcloud-deploytarget) that verifies the$IBMCLOUD_REGISTRY_SECRETexists viaibmcloud ce secret getbefore attempting deployment. If missing, it exits early with actionable instructions and the exact command to create the pull secret.docs/docs/howto/ibm-cloud-code-engine.mdto explicitly include the registry pull secret creation step and added an informational note..env.cevariables (export $(grep -v '^#' .env.ce | grep -v '^$' | xargs)) prior to running manual CLI commands, avoiding empty variable expansion errors.📏 Reviewability
triage🏷️ Type of Change
🧪 Verification
List exact commands, screenshots, videos, logs, reproduction steps, or manual validation. If evidence is not feasible, explain why.
make ruff interrogate pylintmake -n ibmcloud-deploy✅ Checklist
make black isort pre-commit)📓 Notes (optional)
$$(echo $(IBMCLOUD_IMAGE_NAME) | cut -d/ -f1)and escapes\$$IBMCLOUD_API_KEY.