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

Add docker image building targets to Makefile #478

Merged
merged 6 commits into from
Apr 9, 2021

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Apr 8, 2021

Description

Adds make targets to build the docker images.

Why is this needed

Its not very obvious that a user needs to run make crosscompile before they can try to build the docker images (as came up in community slack yesterday).
By having a proper target in the Makefile we might avoid this situation entirely.

How Has This Been Tested?

Ran make images

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

mmlb added 4 commits April 8, 2021 10:42
Its not currently used.

Signed-off-by: Manuel Mendez <[email protected]>
This will hopefully make the "main" targets of the makefile easier
to find/use.

Signed-off-by: Manuel Mendez <[email protected]>
Makefile now just has the targets that a caller is likely to be
interested in, everything else goes into rules.mk. Hopefully
this makes this Makefile easier to use/obvious.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb requested a review from gianarb April 8, 2021 14:50
@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #478 (74b32dc) into master (a7ffd35) will not change coverage.
The diff coverage is n/a.

❗ Current head 74b32dc differs from pull request most recent head 81f568e. Consider uploading reports for the commit 81f568e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #478   +/-   ##
=======================================
  Coverage   32.09%   32.09%           
=======================================
  Files          44       44           
  Lines        3103     3103           
=======================================
  Hits          996      996           
  Misses       2017     2017           
  Partials       90       90           

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 a7ffd35...81f568e. Read the comment docs.

@gianarb
Copy link
Contributor

gianarb commented Apr 8, 2021 via email

@mmlb
Copy link
Contributor Author

mmlb commented Apr 8, 2021

Thanks, when I look at Makefile I am not able to use it. I tried weeks ago but it always requires too much time and I end up writing my own thing... Do you think we can write something as part of the README.md about how to do common tasks like genering all the binaries? single binaries? Does cross compile? docker images and maybe clean the environment? Thanks a lot!! This is mainly a request I personally have, but probably many others have the same issue :D

I added I think the most relevant bits of that to README.md already. I also reworked the Makefile so only the "important" targets are there and also added make help to it (which is also in the README). I think that should be good enough. Can you take a look at those and see if you want something tweaked/added?

@mmlb
Copy link
Contributor Author

mmlb commented Apr 8, 2021

@gianarb I was going to add the cli,server,worker pseudo targets to the Makefile instead of just as virtual deps but meh go compile is fast enough that I don't think it really matters to just build worker or cli or server, especially when make can do it in parallel.

Also clean is just better off with git clean -fxd imo, but I don't like to do that in make.

mmlb added 2 commits April 8, 2021 16:14
This way the Dockerfile is written to take into account only
its current dir for context which minimizes things uploaded to
docker for the build.

Signed-off-by: Manuel Mendez <[email protected]>
Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Apr 9, 2021
@mmlb mmlb merged commit a56e5cf into tinkerbell:master Apr 9, 2021
@mmlb mmlb deleted the build-image-from-make branch April 9, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants