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

Add ';' to separate if statements #1062

Merged
merged 2 commits into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@gdbelvin
Collaborator

gdbelvin commented Oct 9, 2018

The multiple statements inside the bash if block are being treated as one continuous command.
This PR separates them properly with semicolons.

Fixes #1060

@gdbelvin gdbelvin requested a review from AlCutter Oct 9, 2018

Show outdated Hide outdated .travis.yml

@gdbelvin gdbelvin merged commit 9fd8ff9 into google:master Oct 9, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
GolangCI No issues found!
Details
cla/google All necessary CLAs are signed
@AlCutter

This comment has been minimized.

Show comment
Hide comment
@AlCutter

AlCutter Oct 9, 2018

Could you add a comment here outlining why it was ok to merge with failing travis?

AlCutter commented Oct 9, 2018

Could you add a comment here outlining why it was ok to merge with failing travis?

@gdbelvin

This comment has been minimized.

Show comment
Hide comment
@gdbelvin

gdbelvin Oct 9, 2018

Collaborator

Sure thing: While we were waiting for this PR to get merged, google/tink published breaking changes to their repo which are resolved in #1061.

In order to avoid merging the two PRs together and polluting the commit stream, one of them needed to be merged while Travis was unhappy.

Collaborator

gdbelvin commented Oct 9, 2018

Sure thing: While we were waiting for this PR to get merged, google/tink published breaking changes to their repo which are resolved in #1061.

In order to avoid merging the two PRs together and polluting the commit stream, one of them needed to be merged while Travis was unhappy.

@AlCutter

This comment has been minimized.

Show comment
Hide comment
@AlCutter

AlCutter Oct 9, 2018

Thanks - I think it's worthwhile capturing these!

As an aside, would it have been better to merge #1061 to address the broken repo first?
I see that one is blocked on review, but otherwise aren't you going to end up pushing multiple merges which haven't been travis tested until that does go in?

AlCutter commented Oct 9, 2018

Thanks - I think it's worthwhile capturing these!

As an aside, would it have been better to merge #1061 to address the broken repo first?
I see that one is blocked on review, but otherwise aren't you going to end up pushing multiple merges which haven't been travis tested until that does go in?

@gdbelvin gdbelvin deleted the gdbelvin:fix-travis branch Oct 9, 2018

@gdbelvin

This comment has been minimized.

Show comment
Hide comment
@gdbelvin

gdbelvin Oct 9, 2018

Collaborator

I think it was better to merge this one first because it was simpler, and because it was preventing #1061 from being verified by travis. There were two breakages and no single PR could have made the build green, unless we merged the two PRs.

Now that this one is in, #1061 is green and we can proceed as usual with Travis green always required.

Collaborator

gdbelvin commented Oct 9, 2018

I think it was better to merge this one first because it was simpler, and because it was preventing #1061 from being verified by travis. There were two breakages and no single PR could have made the build green, unless we merged the two PRs.

Now that this one is in, #1061 is green and we can proceed as usual with Travis green always required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment