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

Refactor docker/* targets in Makefile #1028

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Refactor docker/* targets in Makefile #1028

merged 2 commits into from
Mar 13, 2024

Conversation

gliptak
Copy link
Contributor

@gliptak gliptak commented Mar 6, 2024

Fixes #647

@gliptak
Copy link
Contributor Author

gliptak commented Mar 8, 2024

@maxfisher-g please review

@maxfisher-g
Copy link
Contributor

Hi @gliptak, thanks for your continued work on this issue!

Looking at the linked issue and example Makefile, I think the 'top level' recipes should be verbs like build, sync, push etc. We probably don't need the docker prefix for everything.

Additionally, can we make it so that

  • make build will build the prod images and sandboxes
  • make push will push the prod images and sandboxes (after building, as currently)
  • make sync will sync all sandboxes (after building, as currently)

Thanks very much!

@gliptak
Copy link
Contributor Author

gliptak commented Mar 11, 2024

updated. please let me know

@maxfisher-g
Copy link
Contributor

maxfisher-g commented Mar 11, 2024

@gliptak nice, thank you!

One more thing, if you look at the original comment in #647, the format of the recipes is like build/image/XXX. This means that make build/image should build all the non-sandbox images and make build/sandbox will build all the sandboxes, etc.

Currently, the recipes in our Makefile look like e.g. build/XXX_image, sync/XXX_sandbox. Are you able to change the format from this to e.g. build/image/XXX, sync/sandbox/XXX?

This would be much appreciated :)

@gliptak
Copy link
Contributor Author

gliptak commented Mar 11, 2024

@maxfisher-g thank you. updated

@maxfisher-g
Copy link
Contributor

Thanks for the update @gliptak!

Could you please add an entry for sync to the output of make help, and then I think we can merge this PR :)

The group targets e.g. make build/image , make sync/sandbox don't work currently, but this can be fixed in another PR.

Signed-off-by: Gábor Lipták <[email protected]>
@gliptak
Copy link
Contributor Author

gliptak commented Mar 12, 2024

@maxfisher-g updated sync

@maxfisher-g
Copy link
Contributor

@gliptak thank you! I'll merge ;)

@maxfisher-g maxfisher-g merged commit c473e2d into ossf:main Mar 13, 2024
10 checks passed
@gliptak gliptak deleted the make2 branch March 13, 2024 18:12
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.

Bring Makefile naming in line with criticality score
2 participants