-
Notifications
You must be signed in to change notification settings - Fork 122
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 confirmation message when building with plugin id "com.mattermost.plugin-starter-template" #156
Conversation
Hello @haardikdharma10, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
@@ -57,6 +57,7 @@ endif | |||
.PHONY: server | |||
server: | |||
ifneq ($(HAS_SERVER),) | |||
$(info Are you sure you want to build the plugin with an id of "com.mattermost.plugin-starter-template"? Consider editing plugin.json to configure your project.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if using $(info message)
is the right approach or should it be changed to $(warning message)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only my system using warning
outputs the line number, which is not so helpful.
Maybe adding a newline after the output helps to separate it from the following log lines.
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Sorry for the delay @haardikdharma10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question here.
@@ -57,6 +57,7 @@ endif | |||
.PHONY: server | |||
server: | |||
ifneq ($(HAS_SERVER),) | |||
$(info Are you sure you want to build the plugin with an id of "com.mattermost.plugin-starter-template"? Consider editing plugin.json to configure your project.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will show the message always, right? I mean... even if I have edited my plugin.json and have the correct id, I will still receive this message. Shouldn't we do a check on the manifest id somehow? Or add something more like "edit the plugin.json and remove this line".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it, because a warning that always appear is a warning you ignore. And the problem will still be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What change would you like to suggest here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like:
ifeq ($(PLUGIN_ID), com.mattermost.plugin-starter-template)
echo "Are you sure you want to build the plugin with an id of "com.mattermost.plugin-starter-template"? Consider editing plugin.json to configure your project."
endif
I would even consider exiting and not allowing the plugin to be compiled, but that may be too drastic :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@haardikdharma10 let us know if you have any questions for the above suggestion?
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
Summary
Fixes: #152