Skip to content

Add makefile#15

Merged
cpretzer merged 5 commits intolinkerd:masterfrom
aliariff:add-makefile
Jun 23, 2020
Merged

Add makefile#15
cpretzer merged 5 commits intolinkerd:masterfrom
aliariff:add-makefile

Conversation

@aliariff
Copy link
Contributor

No description provided.

@cpretzer cpretzer requested a review from alpeb June 17, 2020 17:40
@cpretzer
Copy link
Contributor

This should be merged before #12 per @aliariff

@cpretzer
Copy link
Contributor

cpretzer commented Jun 18, 2020

@alpeb Can you have a look at this once #3 is merged?

I chatted with @aliariff about the change and the reasoning behind opting for a Makefile. It's a great way to conform to how builds are done in the other repositories.

Once #3 is merged, @aliariff will rebase this to fix the lint errors

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

make makes sense to me 😄
Is there a way to add some kind of help string or something into the Makefile to advise people the series of targets to be run (e.g. build, kind-load, integration-test)? I understand we shouldn't make those dependent on one another given this wouldn't necessary be run in KinD, but some way of stating that dependency on the KinD scenario would be helpful.

@aliariff
Copy link
Contributor Author

@alpeb I add some help text. Is that ok?

image

Makefile Outdated

.PHONY: help
help: ## Show this help
@echo 'Info: For integration test using KinD, run: make kind-load && make integration-test'
Copy link
Member

Choose a reason for hiding this comment

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

Awesome! I believe make kind-load integration-test is enough, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, you are right

Makefile Outdated
.PHONY: help
help: ## Show this help
@echo 'Info: For integration test using KinD, run: make kind-load && make integration-test'
@echo 'Info: For integration test using minikube, run: make integration-test'
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just say something like "For other environments, run make integration-test after having uploaded the images" given this might be run in other places like cloud environments.

help: ## Show this help
@echo 'Info: For integration test using KinD, run: make kind-load && make integration-test'
@echo 'Info: For integration test using minikube, run: make integration-test'
@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
Copy link
Member

Choose a reason for hiding this comment

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

Loved this technique, I'm gonna steal it 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpretzer
Copy link
Contributor

@aliariff would you mind rebasing from master so that the lint checks pass? We should be able to merge after that

aliariff added 5 commits June 23, 2020 20:57
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Signed-off-by: Ali Ariff <ali.ariff12@gmail.com>
Copy link
Contributor

@cpretzer cpretzer left a comment

Choose a reason for hiding this comment

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

Great work!

@cpretzer cpretzer merged commit 37ef901 into linkerd:master Jun 23, 2020
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.

3 participants