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

Adds help command #1622

Merged
merged 3 commits into from
Oct 14, 2021
Merged

Conversation

hasukmistry
Copy link
Contributor

Introduces help command in makefile. Adds comments along with make targets to display in make help section.

@samhita-alla
Copy link
Contributor

samhita-alla commented Oct 12, 2021

Thank you for creating this PR!

@kumare3 / @EngHabu / @evalsocket, can you let me know if any other changes need to be made in the Makefiles or configuration files? If there are any, and if @hasukmistry is willing to take up those, I can approve this PR for Hacktoberfest.

yindia
yindia previously approved these changes Oct 12, 2021
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

Thank you for your contributions. Left just a couple of suggestions and comments..

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
.PHONY: end2end
end2end:
end2end: ## Launch dockernetes and execute tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is deprecated... Can you check if it's used throughout the repo. And if not, please delete the target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this script end2end/launch_dockernetes.sh is still in place. I am not sure about it. I can remove target if you want me to :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The file exists, but I don't think end2end/launch_dockernetes.sh is being used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samhita-alla so can i remove the target?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

@evalsocket this target isn't surely required, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's deprecated.

Makefile Outdated Show resolved Hide resolved

.PHONY: help
help: SHELL := /bin/sh
help: ## List available commands and their usage
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on vanilla OSX and Linux with no additional tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested this with Ubuntu, Mac Os without installing any additional tool.

Hasmukh K Mistry added 2 commits October 13, 2021 12:31
Introduces help command in makefile. Adds comments along with make targets to display in make help section.

Signed-off-by: Hasmukh K Mistry <[email protected]>
Makes changes based on suggestions

Signed-off-by: Hasmukh K Mistry <[email protected]>
@hasukmistry hasukmistry force-pushed the feature/make-help-command branch from 91626e1 to 533cc08 Compare October 13, 2021 07:02
Removes unused target from makefile

Signed-off-by: Hasmukh K Mistry <[email protected]>
@hasukmistry
Copy link
Contributor Author

can we merge this PR?

@samhita-alla samhita-alla merged commit b325cb0 into flyteorg:master Oct 14, 2021
@welcome
Copy link

welcome bot commented Oct 14, 2021

Congrats on merging your first pull request! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants