-
Notifications
You must be signed in to change notification settings - Fork 82
OADP CLI check - do not attempt to connect to server #1904
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: oadp-dev
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mpryc 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 |
Use `--client-only` with `kubectl oadp version` in the CLI availability check to ensure it only reports the client version without attempting to connect to the cluster. Signed-off-by: Michal Pryc <[email protected]>
cbf5124
to
8de0f08
Compare
@@ -198,7 +198,7 @@ var _ = ginkgo.Describe("Backup and restore tests via OADP CLI", ginkgo.Label("c | |||
ginkgo.BeforeAll(func() { | |||
// Verify OADP CLI is available (should be installed in Docker image) | |||
log.Print("Verifying OADP CLI is available...") | |||
cmd := exec.Command("kubectl", "oadp", "version") | |||
cmd := exec.Command("kubectl", "oadp", "version", "--client-only") |
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.
It's how currently oadp client is implemented but IMO the flag should be --client
to match what is in the 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.
It's more a parallel to how velero CLI is doing, not exactly 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.
I think keep it same as velero CLI, and propose velero CLI upstream match kubectl in the future.
@mpryc: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
From reviewing recent CLI e2e PR, failures are from a path issue and solved in #1899. Even with velero version trying to connect to server, it is only printing as error, but is not the root cause of recent cli CI failures. We can leave this PR open for discussion if we rather this check be purely client-only for cleaner logs/semantics. |
PR needs rebase. 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 kubernetes-sigs/prow repository. |
Use
--client
withkubectl oadp version
in the CLI availability check to ensure it only reports the client version without attempting to connect to the cluster. This prevents errors when thevelero
namespace is not yet present during test setup.The example error:
https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oadp-operator/1897/pull-ci-openshift-oadp-operator-oadp-dev-4.19-e2e-test-cli-aws/1955901235508809728/artifacts/e2e-test-cli-aws/e2e/build-log.txt