mattermost / mattermost-plugin-starter-template

Build scripts and templates for writing Mattermost plugins.
https://developers.mattermost.com/extend/plugins/
Apache License 2.0
133 stars 121 forks source link

Various tooling improvements from other plugins #96

Closed lieut-data closed 4 years ago

lieut-data commented 4 years ago

Summary

Upstream various changes from other plugins:

Fixes: https://mattermost.atlassian.net/browse/MM-11867

lieut-data commented 4 years ago

Skipping 3: QA Review, since only tooling changes.

lieut-data commented 4 years ago

@cpoile, my hope is to downstream this back to incident response, with just one additional change in build/custom.mk, otherwise having Makefile be 100% a match for the starter-template.

I took the opportunity to slim out some of the code in incident-response around copying plugins instead of Client4-installing them, since GOPATH is long dead, and the mmctl strategy is fast becoming the popular approach. I also took some opinionated strategies with target naming, also removing the background nature of attach-headless and forcing you to Ctrl-C it instead.

hanzei commented 4 years ago

@lieut-data is there any urgency with reviewing this PR? I would love to understand the details before giving a review, but this will take time.

lieut-data commented 4 years ago

@hanzei, there's no immediate hurry -- I can propose the same changes into incident response in parallel, and if we diverge, even up later.

mickmister commented 4 years ago

I apparently missed this PR and have a similar effort at https://github.com/mattermost/mattermost-plugin-starter-template/pull/99 . I got most of the inspiration from the Incident Response plugin as it has a pretty robust dev setup. Two additions I've made:

image

I mainly made the PR to get some eyes on it before holistically applying the additions to the other plugin repos. It would be immensely helpful to get all of the plugins up to speed on these configurations!

I know there is a task to have various aspects of the repos automatically synchronized, but I think this first go-through should be done manually as there are some intricacies to make sure it is all configured correctly.

mickmister commented 4 years ago

Another good thing to add would be an .npmrc file at the root of the webapp folder, that enforces dependencies to be installed with save-exact.

lieut-data commented 4 years ago

@mickmister, I've gone ahead and made the following changes:

Can you and @cpoile give this a whirl and see if it fits your workflows / make intuitive sense?

mickmister commented 4 years ago

@lieut-data I'm not sure I see much benefit from using make watch without DEBUG=1, so it seems off to me to have non-debug to be the default. I don't see why I would want the watch behavior, without the benefits of the debug build, but I suppose I understand if it is desirable to be able to replicate the production build in rapid development.

Regardless of that point, I do like the explicit mention of watch in the command.

lieut-data commented 4 years ago

I suppose I understand if it is desirable to be able to replicate the production build in rapid development

Yeah, this was exactly my imagined use case. I've often had to debug issues that didn't manifest in debug builds, but could be reproduce in production builds. By exporting DEBUG=1 to your environment, you could get the desired default in most cases. Actually, this reminds me that I haven't updated the README.md to document all this -- will do before merging.

mickmister commented 4 years ago

I think a make disable command would be great to include here. It would do the same thing make reset does, but without the final enable. I've just had the need for one and it seems natural to use.

mickmister commented 4 years ago

I'd also like to mention an open PR https://github.com/mattermost/mattermost-plugin-starter-template/pull/86 that is meant to synchronize changes like the ones in this tooling improvements PR. I'd like to make sure we synchronize on upgrading all things that the PR supports, after this one is merged. The PR is stale, but I've pinged the contributor to see how he's doing.

lieut-data commented 4 years ago

Thanks, @mickmister! I've updated with your excellent suggestions.