-
Notifications
You must be signed in to change notification settings - Fork 140
Sync with playbooks: install-go-tools, gotestsum, and dynamic versions #192
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
Changes from all commits
6158e72
5463bc1
f927586
42f5eaf
0ac4026
0902371
14a8271
ef17405
1767c29
175bc99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| server/manifest.go linguist-generated=true | ||
| webapp/src/manifest.js linguist-generated=true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,13 @@ | ||
| bin/ | ||
| dist/ | ||
| webapp/src/manifest.ts | ||
| server/manifest.go | ||
|
|
||
| # Mac | ||
| .DS_Store | ||
|
|
||
| # Jetbrains | ||
| .idea/ | ||
|
|
||
| # VS Code | ||
| .vscode |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,6 @@ GO ?= $(shell command -v go 2> /dev/null) | |||||
| NPM ?= $(shell command -v npm 2> /dev/null) | ||||||
| CURL ?= $(shell command -v curl 2> /dev/null) | ||||||
| MM_DEBUG ?= | ||||||
| MANIFEST_FILE ?= plugin.json | ||||||
| GOPATH ?= $(shell go env GOPATH) | ||||||
| GO_TEST_FLAGS ?= -race | ||||||
| GO_BUILD_FLAGS ?= | ||||||
|
|
@@ -13,6 +12,10 @@ DEFAULT_GOARCH := $(shell go env GOARCH) | |||||
|
|
||||||
| export GO111MODULE=on | ||||||
|
|
||||||
| # We need to export GOBIN to allow it to be set | ||||||
| # for processes spawned from the Makefile | ||||||
| export GOBIN ?= $(PWD)/bin | ||||||
|
|
||||||
| # You can include assets this directory into the bundle. This can be e.g. used to include profile pictures. | ||||||
| ASSETS_DIR ?= assets | ||||||
|
|
||||||
|
|
@@ -22,7 +25,6 @@ default: all | |||||
|
|
||||||
| # Verify environment, and define PLUGIN_ID, PLUGIN_VERSION, HAS_SERVER and HAS_WEBAPP as needed. | ||||||
| include build/setup.mk | ||||||
| include build/legacy.mk | ||||||
|
|
||||||
| BUNDLE_NAME ?= $(PLUGIN_ID)-$(PLUGIN_VERSION).tar.gz | ||||||
|
|
||||||
|
|
@@ -41,24 +43,34 @@ endif | |||||
| .PHONY: all | ||||||
| all: check-style test dist | ||||||
|
|
||||||
| ## Propagates plugin manifest information into the server/ and webapp/ folders. | ||||||
| .PHONY: apply | ||||||
| apply: | ||||||
| ./build/bin/manifest apply | ||||||
|
|
||||||
| ## Install go tools | ||||||
| install-go-tools: | ||||||
| @echo Installing go tools | ||||||
| $(GO) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.1 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually need to install the binary? Or is
Suggested change
enough?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the added duration comes from checking the remote go modules DB. The increase is similar running One downside of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you're right: I wasn't comparing apples to apples as I thought! |
||||||
| $(GO) install gotest.tools/gotestsum@v1.7.0 | ||||||
|
Comment on lines
+51
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like that plugin now own the version of |
||||||
|
|
||||||
| ## Runs eslint and golangci-lint | ||||||
| .PHONY: check-style | ||||||
| check-style: webapp/node_modules | ||||||
| check-style: apply webapp/node_modules install-go-tools | ||||||
| @echo Checking for style guide compliance | ||||||
|
|
||||||
| ifneq ($(HAS_WEBAPP),) | ||||||
| cd webapp && npm run lint | ||||||
| cd webapp && npm run check-types | ||||||
| endif | ||||||
|
|
||||||
| # It's highly recommended to run go-vet first | ||||||
| # to find potential compile errors that could introduce | ||||||
| # weird reports at golangci-lint step | ||||||
| ifneq ($(HAS_SERVER),) | ||||||
| @if ! [ -x "$$(command -v golangci-lint)" ]; then \ | ||||||
| echo "golangci-lint is not installed. Please see https://github.com/golangci/golangci-lint#install for installation instructions."; \ | ||||||
| exit 1; \ | ||||||
| fi; \ | ||||||
|
|
||||||
| @echo Running golangci-lint | ||||||
| golangci-lint run ./... | ||||||
| $(GO) vet ./... | ||||||
| $(GOBIN)/golangci-lint run ./... | ||||||
| endif | ||||||
|
|
||||||
| ## Builds the server, if it exists, for all supported architectures, unless MM_SERVICESETTINGS_ENABLEDEVELOPER is set. | ||||||
|
|
@@ -104,7 +116,7 @@ endif | |||||
| bundle: | ||||||
| rm -rf dist/ | ||||||
| mkdir -p dist/$(PLUGIN_ID) | ||||||
| cp $(MANIFEST_FILE) dist/$(PLUGIN_ID)/ | ||||||
| ./build/bin/manifest dist | ||||||
| ifneq ($(wildcard $(ASSETS_DIR)/.),) | ||||||
| cp -r $(ASSETS_DIR) dist/$(PLUGIN_ID)/ | ||||||
| endif | ||||||
|
|
@@ -125,7 +137,7 @@ endif | |||||
|
|
||||||
| ## Builds and bundles the plugin. | ||||||
| .PHONY: dist | ||||||
| dist: server webapp bundle | ||||||
| dist: apply server webapp bundle | ||||||
|
|
||||||
| ## Builds and installs the plugin to a server. | ||||||
| .PHONY: deploy | ||||||
|
|
@@ -134,7 +146,7 @@ deploy: dist | |||||
|
|
||||||
| ## Builds and installs the plugin to a server, updating the webapp automatically when changed. | ||||||
| .PHONY: watch | ||||||
| watch: server bundle | ||||||
| watch: apply server bundle | ||||||
| ifeq ($(MM_DEBUG),) | ||||||
| cd webapp && $(NPM) run build:watch | ||||||
| else | ||||||
|
|
@@ -188,17 +200,28 @@ detach: setup-attach | |||||
|
|
||||||
| ## Runs any lints and unit tests defined for the server and webapp, if they exist. | ||||||
| .PHONY: test | ||||||
| test: webapp/node_modules | ||||||
| test: apply webapp/node_modules install-go-tools | ||||||
| ifneq ($(HAS_SERVER),) | ||||||
| $(GOBIN)/gotestsum -- -v ./... | ||||||
| endif | ||||||
| ifneq ($(HAS_WEBAPP),) | ||||||
| cd webapp && $(NPM) run test; | ||||||
| endif | ||||||
|
|
||||||
| ## Runs any lints and unit tests defined for the server and webapp, if they exist, optimized | ||||||
| ## for a CI environment. | ||||||
| .PHONY: test-ci | ||||||
| test-ci: apply webapp/node_modules install-go-tools | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a follow up ticket to actually use the target in CI?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not at the moment, but I'm a bit blurry on how to achieve that -- it looks like we import
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it's time to move to hand-picked CI versions. We have been bitten by the auto-update of @main way too often. Espcecially, updates to cc @mickmister
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mickmister Would you be open to owning that change? Brightscout could surely help with executing it. |
||||||
| ifneq ($(HAS_SERVER),) | ||||||
| $(GO) test -v $(GO_TEST_FLAGS) ./server/... | ||||||
| $(GOBIN)/gotestsum --format standard-verbose --junitfile report.xml -- ./... | ||||||
| endif | ||||||
| ifneq ($(HAS_WEBAPP),) | ||||||
| cd webapp && $(NPM) run test; | ||||||
| endif | ||||||
|
|
||||||
| ## Creates a coverage report for the server code. | ||||||
| .PHONY: coverage | ||||||
| coverage: webapp/node_modules | ||||||
| coverage: apply webapp/node_modules | ||||||
| ifneq ($(HAS_SERVER),) | ||||||
| $(GO) test $(GO_TEST_FLAGS) -coverprofile=server/coverage.txt ./server/... | ||||||
| $(GO) tool cover -html=server/coverage.txt | ||||||
|
|
||||||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,7 @@ | ||
| import {manifest} from './manifest'; | ||
| import manifest from './manifest'; | ||
|
|
||
| test('Plugin manifest, id and version are defined', () => { | ||
| expect(manifest).toBeDefined(); | ||
| expect(manifest.id).toBeDefined(); | ||
| expect(manifest.version).toBeDefined(); | ||
| }); | ||
|
|
||
| // To ease migration, verify separate export of id and version. | ||
| test('Plugin id and version are defined', () => { | ||
| expect(manifest.id).toBeDefined(); | ||
| expect(manifest.version).toBeDefined(); | ||
| }); |

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 is the impact of this change on CI? AFAIK it also installs a version of
golangci-lint. Can we drop that?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.
See below: uncertain how to roll out these changes to CI without breaking everything. For now, this at least gives the developer a reliable definition of the version in play. Thoughts?