Skip to content
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

xds interop: Fix buildscripts not continuing on a failed test suite #2323

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

sergiitk
Copy link
Member

Apparently there's a difference between bash 3 and bash 4. OSX comes with bash 3 out-of-box, so for whoever wrote this logic it "worked on my machine".

Apparently there's a difference between bash 3 and bash 4.
OSX comes with bash 3 out-of-box, so for whoever wrote this logic
it "worked on my machine".
@sergiitk
Copy link
Member Author

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

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

I think I understand why the current code might not be working: the (( construct returns a 0 exit code if the value is non-zero. Since the value starts at 0 and we do a post-increment, it will always fail the first time. If that is the problem, changing it to a pre-increment should fix it.

I don't understand how adding && true could ever possibly do anything. It's like writing && true in other languages.

@sergiitk
Copy link
Member Author

sergiitk commented Jan 13, 2023

&& true ensures the exit code of this op is always true. It's like || true used in bash to suppress an exit of command that failed. https://stackoverflow.com/questions/11231937/bash-ignoring-error-for-a-particular-command

In this case, we're doing the same, just after the increment op.

But good idea with the pre-increment. I'll give it a try.

@sergiitk
Copy link
Member Author

Yep, you're right. Pre-increment works. However, the && true change's already merged to the other repos. @murgatroid99 Would you be ok accepting this as is? Or you feel strongly about it?

Copy link
Member Author

@sergiitk sergiitk left a comment

Choose a reason for hiding this comment

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

Address the feedback: use pre-increment instead of && true

EDIT: this was supposed to be the commit message

@murgatroid99 murgatroid99 merged commit 3bf1409 into grpc:master Jan 17, 2023
@sergiitk sergiitk deleted the xds-interop-fix-buildscript-suites branch January 17, 2023 19:06
sergiitk added a commit to grpc/grpc-java that referenced this pull request Jan 17, 2023
… suite (#9817)" (#9831)

This reverts commit d83a599.

Reverted in favor of better syntax suggested here: grpc/grpc-node#2323 (review). This fix will be sent as another PR for the convenience of backporting.
sergiitk pushed a commit to sergiitk/grpc-node that referenced this pull request Jan 18, 2023
…t-suites

xds interop: Fix buildscripts not continuing on a failed test suite
sergiitk pushed a commit to sergiitk/grpc-node that referenced this pull request Jan 18, 2023
…t-suites

xds interop: Fix buildscripts not continuing on a failed test suite
sergiitk pushed a commit to sergiitk/grpc-node that referenced this pull request Jan 18, 2023
…t-suites

xds interop: Fix buildscripts not continuing on a failed test suite
sergiitk pushed a commit to sergiitk/grpc-node that referenced this pull request Jan 18, 2023
…t-suites

xds interop: Fix buildscripts not continuing on a failed test suite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants