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

build: Fix shell syntax #62

Merged
merged 1 commit into from
Jul 11, 2017
Merged

build: Fix shell syntax #62

merged 1 commit into from
Jul 11, 2017

Conversation

abeaumont
Copy link
Contributor

No description provided.

@abeaumont abeaumont self-assigned this Jul 11, 2017
@abeaumont abeaumont requested a review from juanjux July 11, 2017 10:16
@codecov
Copy link

codecov bot commented Jul 11, 2017

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   61.79%   61.79%           
=======================================
  Files          14       14           
  Lines         746      746           
=======================================
  Hits          461      461           
  Misses        244      244           
  Partials       41       41

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 412e537...43e3a5c. Read the comment docs.

@@ -142,6 +142,6 @@ push: docker-image-build
$(DOCKER_PUSH) $(call unescape_docker_tag,$(DOCKER_IMAGE_VERSIONED))
@if [ "$$TRAVIS_TAG" != "" ]; then \
$(DOCKER_TAG) $(call unescape_docker_tag,$(DOCKER_IMAGE_VERSIONED)) \
$(call unescape_docker_tag,$(DOCKER_IMAGE)):latest
$(DOCKER_PUSH) $(call unescape_docker_tag,$(DOCKER_IMAGE):latest)
$(call unescape_docker_tag,$(DOCKER_IMAGE)):latest; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this ; be an &&?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be both, I used ; because:

  • it's the way it was, and has worked fine so far
  • it'll fail eitherwise
  • it may give a false sensation of having some kind of transactional support, which we don't, and I don't think it's worth at this moment.

I'm not strongly against it though, so I can change it if you prefer.

Copy link
Contributor

@juanjux juanjux Jul 11, 2017

Choose a reason for hiding this comment

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

I'm not strong against it either, just commenting the usual way to write those commands in Dockerfiles. Your arguments make sense and the only advantage of the && would be that the script would fail at the specific point of failure (and not one command later) so leave it with the ; if you want.

@abeaumont abeaumont merged commit 14fcc9c into bblfsh:master Jul 11, 2017
@abeaumont abeaumont deleted the feature/push-an-image-per-commit branch July 11, 2017 12:06
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.

2 participants