-
Notifications
You must be signed in to change notification settings - Fork 2.1k
openstack: Relax script checks #6727
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
Conversation
The double square bracket is not POSIX-sh
|
/uncc jcpowermac mtnbikenc /cc mandre tomassedovic Fedosin |
There are unbound variables in the script; they have to be fixed before we set the unbound variable check.
462be68 to
3a4904f
Compare
|
/lgtm I'm putting this on hold to make sure we verify it fixes the OpenStack job. This change (seemingly) introduced the issue and it showed up in the job, but it got merged anyway: Anyone feel free to remove the |
|
/lgtm Hmm, there's something that escape me. The release/ci-operator/templates/openshift/installer/cluster-launch-installer-openstack-e2e.yaml Line 24 in 3a4904f
However, without this patch, it was complaining that the variable was unbound: I agree with the proposed fix: rather than chasing all unbound variables, let's drop the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mandre, pierreprinetti, tomassedovic 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 |
|
I agree with @mandre though I would like us to forbid unbound variables eventually. Let's merge this since it's breaking the OpenStack CI and then investigate this properly. |
|
/test pj-rehearse |
Of course, I should have said "chasing all unbound variables now" - removing the |
|
/close in favour of #6728 |
|
@pierreprinetti: Closed this PR. 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 kubernetes/test-infra repository. |
|
@pierreprinetti: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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/test-infra repository. I understand the commands that are listed here. |
There are unbound variables in the script; they have to be fixed before
we set the unbound variable check.
Additionally, use
bashin the setup script, as bash syntax is used.ref.: #6725