Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Ensure invocation image has been build before running make command #631

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

glours
Copy link
Contributor

@glours glours commented Sep 23, 2019

As a potential contributor of Docker App
I want to run make or make all on docker-app source code without having errors during the end-to-end tests phase

- What I did
Add a build of the invocation image if needed
- How I did it
Check if the image is already present & if not build it
- How to verify it

  • remove all the docker/cnab-app-base images
  • run a make command
  • check the image is building before executing tests
  • re-run a make command
  • check the image isn't build again

- A picture of a cute animal (not mandatory but encouraged)

bear

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, even if I'm not a Makefile expert 😅
Are you sure re-invoking make like this is the best way?

@glours
Copy link
Contributor Author

glours commented Sep 23, 2019

I'm not a Makefile expert too 😅

Makefile Outdated
@@ -72,6 +72,10 @@ bin/$(BIN_NAME)-%.exe bin/$(BIN_NAME)-%: cmd/$(BIN_NAME) check_go_env
bin/%: cmd/% check_go_env
$(GO_BUILD) -o $@$(EXEC_EXT) ./$<

build-invocation-image:
@echo "Build invocation image if needed"
$(if $(shell docker images -q docker/cnab-app-base:$(TAG)),, make -f ./docker.Makefile invocation-image)
Copy link
Member

Choose a reason for hiding this comment

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

You can replace make by $(MAKE).
And just for readability it can be multi lines.

Suggested change
$(if $(shell docker images -q docker/cnab-app-base:$(TAG)),, make -f ./docker.Makefile invocation-image)
$(if $(shell docker images -q docker/cnab-app-base:$(TAG)),, \
$(MAKE) -f ./docker.Makefile invocation-image \
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks 👍

@ndeloof
Copy link
Contributor

ndeloof commented Sep 24, 2019

I'm not a Makefile expert too sweat_smile

nobody is

@glours glours force-pushed the suitable_error_4_unknown_command branch from 7dfef1d to 85111ce Compare September 24, 2019 07:35
@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@b118ccd). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #631   +/-   ##
========================================
  Coverage          ?   72.4%           
========================================
  Files             ?      49           
  Lines             ?    2577           
  Branches          ?       0           
========================================
  Hits              ?    1866           
  Misses            ?     477           
  Partials          ?     234

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 b118ccd...38b3c12. Read the comment docs.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Noice

@glours glours force-pushed the suitable_error_4_unknown_command branch from 85111ce to c4292a8 Compare September 24, 2019 15:29
@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "suitable_error_4_unknown_command" git@github.com:glours/app.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354506960
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

…l command

Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
@glours glours force-pushed the suitable_error_4_unknown_command branch from 44db281 to 38b3c12 Compare September 25, 2019 07:41
@ndeloof ndeloof merged commit 54710d5 into docker:master Sep 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants