-
Notifications
You must be signed in to change notification settings - Fork 127
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
Sync with playbooks: install-go-tools, gotestsum, and dynamic versions #192
Changes from 5 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 | ||||||
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? 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. 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 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
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 | ||||||
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? 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 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 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.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,11 +4,50 @@ import ( | |
"encoding/json" | ||
"fmt" | ||
"os" | ||
"strings" | ||
|
||
"github.com/mattermost/mattermost/server/public/model" | ||
"github.com/pkg/errors" | ||
) | ||
|
||
const pluginIDGoFileTemplate = `// This file is automatically generated. Do not modify it manually. | ||
|
||
package main | ||
|
||
import ( | ||
"encoding/json" | ||
"strings" | ||
|
||
"github.com/mattermost/mattermost/server/public/model" | ||
) | ||
|
||
var manifest *model.Manifest | ||
|
||
const manifestStr = ` + "`" + ` | ||
%s | ||
` + "`" + ` | ||
|
||
func init() { | ||
_ = json.NewDecoder(strings.NewReader(manifestStr)).Decode(&manifest) | ||
} | ||
` | ||
|
||
const pluginIDJSFileTemplate = `// This file is automatically generated. Do not modify it manually. | ||
|
||
const manifest = JSON.parse(` + "`" + ` | ||
%s | ||
` + "`" + `); | ||
|
||
export default manifest; | ||
` | ||
|
||
// These build-time vars are read from shell commands and populated in ../setup.mk | ||
var ( | ||
BuildHashShort string | ||
BuildTagLatest string | ||
BuildTagCurrent string | ||
) | ||
|
||
func main() { | ||
if len(os.Args) <= 1 { | ||
panic("no cmd specified") | ||
|
@@ -37,6 +76,16 @@ func main() { | |
fmt.Printf("true") | ||
} | ||
|
||
case "apply": | ||
if err := applyManifest(manifest); err != nil { | ||
panic("failed to apply manifest: " + err.Error()) | ||
} | ||
|
||
case "dist": | ||
if err := distManifest(manifest); err != nil { | ||
panic("failed to write manifest to dist directory: " + err.Error()) | ||
} | ||
|
||
default: | ||
panic("unrecognized command: " + cmd) | ||
} | ||
|
@@ -62,6 +111,30 @@ func findManifest() (*model.Manifest, error) { | |
return nil, errors.Wrap(err, "failed to parse manifest") | ||
} | ||
|
||
// Update the manifest based on the state of the current commit | ||
// Use the first version we find (to prevent causing errors) | ||
var version string | ||
tags := strings.Fields(BuildTagCurrent) | ||
for _, t := range tags { | ||
if strings.HasPrefix(t, "v") { | ||
version = t | ||
break | ||
} | ||
} | ||
if version == "" { | ||
if BuildTagLatest != "" { | ||
version = BuildTagLatest + "+" + BuildHashShort | ||
} else { | ||
version = "v0.0.0+" + BuildHashShort | ||
} | ||
} | ||
manifest.Version = strings.TrimPrefix(version, "v") | ||
hanzei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Update the release notes url to point at the latest tag, if present. | ||
if BuildTagLatest != "" { | ||
manifest.ReleaseNotesURL = manifest.HomepageURL + "releases/tag/" + BuildTagLatest | ||
} | ||
|
||
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. LGTM 👍 If I understand correctly:
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. 💯 |
||
return &manifest, nil | ||
} | ||
|
||
|
@@ -74,3 +147,63 @@ func dumpPluginID(manifest *model.Manifest) { | |
func dumpPluginVersion(manifest *model.Manifest) { | ||
fmt.Printf("%s", manifest.Version) | ||
} | ||
|
||
// applyManifest propagates the plugin_id into the server and webapp folders, as necessary | ||
func applyManifest(manifest *model.Manifest) error { | ||
if manifest.HasServer() { | ||
// generate JSON representation of Manifest. | ||
manifestBytes, err := json.MarshalIndent(manifest, "", " ") | ||
if err != nil { | ||
return err | ||
} | ||
manifestStr := string(manifestBytes) | ||
|
||
// write generated code to file by using Go file template. | ||
if err := os.WriteFile( | ||
"server/manifest.go", | ||
[]byte(fmt.Sprintf(pluginIDGoFileTemplate, manifestStr)), | ||
0600, | ||
); err != nil { | ||
return errors.Wrap(err, "failed to write server/manifest.go") | ||
} | ||
} | ||
|
||
if manifest.HasWebapp() { | ||
// generate JSON representation of Manifest. | ||
// JSON is very similar and compatible with JS's object literals. so, what we do here | ||
// is actually JS code generation. | ||
manifestBytes, err := json.MarshalIndent(manifest, "", " ") | ||
if err != nil { | ||
return err | ||
} | ||
manifestStr := string(manifestBytes) | ||
|
||
// Escape newlines | ||
manifestStr = strings.ReplaceAll(manifestStr, `\n`, `\\n`) | ||
|
||
// write generated code to file by using JS file template. | ||
if err := os.WriteFile( | ||
"webapp/src/manifest.ts", | ||
[]byte(fmt.Sprintf(pluginIDJSFileTemplate, manifestStr)), | ||
0600, | ||
); err != nil { | ||
return errors.Wrap(err, "failed to open webapp/src/manifest.js") | ||
lieut-data marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// distManifest writes the manifest file to the dist directory | ||
func distManifest(manifest *model.Manifest) error { | ||
manifestBytes, err := json.MarshalIndent(manifest, "", " ") | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := os.WriteFile(fmt.Sprintf("dist/%s/plugin.json", manifest.Id), manifestBytes, 0600); err != nil { | ||
return errors.Wrap(err, "failed to write plugin.json") | ||
} | ||
|
||
return nil | ||
} |
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(); | ||
}); |
This file was deleted.
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?