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

chore(mk): move accessible targets to the "make main file" i.e. Makefile #3017

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johncowen
Copy link
Contributor

Changes our approach to Makefile targets to make them more customisable upstream


The biggest problem here is that Make make its troublesome to simply override make targets.

Previously we would 'override' targets by not including the file that had the target we had to override, and then including a different file with our customisations in it for the relevant target. The thing is you would also need to override (or just copy) all targets in a file, rather than just the one you wanted to override.

This approach names all consumable make targets prefixed with a . i.e. .install vs install. We than add references to all these in our base Makefile (essentially makes main file). The Makefile is specific to each project so we can add what we want in there and it doesn't affect anything downstream. This also includes adding different documentation if need be.

For example, say in one make file we have:

install: .install ## Docs for this here

And we need to add some sort of auth to that we can use

.install/auth:
  $(AUTH) $(MAKE) .install
install: .install/auth ## Different docs for this here

Both of the above examples will run our installation scripts without them being duplicated, but one is decorated with some sort of authentication before doing so.

Moving forwards we should add all "common" user scripts in this file using the same approach. We should avoid mixing the documented aliases in with the rest of the ./mk files, and avoid mixing long form scripts into the Makefile itself. I've added a small comment at the top of the Makefile to this effect.

Lastly, I've only moved previously documented targets into this form as these are the commonly used ones. We can move others later on if necessary/desirable.

Signed-off-by: John Cowen <john.cowen@konghq.com>
@johncowen johncowen requested a review from a team as a code owner September 30, 2024 13:16
@johncowen johncowen self-assigned this Sep 30, 2024
Copy link

netlify bot commented Sep 30, 2024

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 8ab27fe
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/66faa49db175500008ef3f67
😎 Deploy Preview https://deploy-preview-3017--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@slonka slonka left a comment

Choose a reason for hiding this comment

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

1 Q

mk/test.mk Show resolved Hide resolved
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