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

Sync with playbooks: install-go-tools, gotestsum, and dynamic versions #192

Merged
merged 10 commits into from
Jan 23, 2024
2 changes: 2 additions & 0 deletions .gitattributes
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
6 changes: 6 additions & 0 deletions .gitignore
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
53 changes: 38 additions & 15 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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 ?=
Expand All @@ -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

Expand All @@ -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

Expand All @@ -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
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to install the binary? Or is

Suggested change
$(GO) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.1
$(GO) run github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.1

enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that would be convenient! But unfortunately much slower it seems?

CleanShot 2024-01-23 at 09 47 36@2x

Copy link
Contributor

Choose a reason for hiding this comment

The 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 go install.

One downside of go run is, that it errors if the machine is offline.

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

@hanzei hanzei Dec 18, 2023

Choose a reason for hiding this comment

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

I really like that plugin now own the version of golangci-lint.


## 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.
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 @main, which would make whatever changes we add there affect all plugins at the same time, vs. plugins opting in to updated CI incrementally. Not sure how to crack this nut: thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 golangci-lint caused issues in the past.

cc @mickmister

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
3 changes: 0 additions & 3 deletions build/legacy.mk

This file was deleted.

133 changes: 133 additions & 0 deletions build/manifest/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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)
}
Expand All @@ -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")

// Update the release notes url to point at the latest tag, if present.
if BuildTagLatest != "" {
manifest.ReleaseNotesURL = manifest.HomepageURL + "releases/tag/" + BuildTagLatest
}

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

If I understand correctly:

  • if the current commit matches a tag, then we use that
  • If not, use the latest tag and append the latest commit hash
  • If no tag available, use v0.0.0 and append the latest commit hash

Copy link
Member Author

Choose a reason for hiding this comment

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

💯

return &manifest, nil
}

Expand All @@ -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")
}
}

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
}
7 changes: 6 additions & 1 deletion build/setup.mk
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ ifeq ($(GO),)
$(error "go is not available: see https://golang.org/doc/install")
endif

# Gather build variables to inject into the manifest tool
BUILD_HASH_SHORT = $(shell git rev-parse --short HEAD)
BUILD_TAG_LATEST = $(shell git describe --tags --match 'v*' --abbrev=0)
BUILD_TAG_CURRENT = $(shell git tag --points-at HEAD)

# Ensure that the build tools are compiled. Go's caching makes this quick.
$(shell cd build/manifest && $(GO) build -o ../bin/manifest)
$(shell cd build/manifest && $(GO) build -ldflags '-X "main.BuildHashShort=$(BUILD_HASH_SHORT)" -X "main.BuildTagLatest=$(BUILD_TAG_LATEST)" -X "main.BuildTagCurrent=$(BUILD_TAG_CURRENT)"' -o ../bin/manifest)

# Ensure that the deployment tools are compiled. Go's caching makes this quick.
$(shell cd build/pluginctl && $(GO) build -o ../bin/pluginctl)
Expand Down
18 changes: 0 additions & 18 deletions plugin.go

This file was deleted.

2 changes: 1 addition & 1 deletion webapp/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {Store, Action} from 'redux';

import {GlobalState} from '@mattermost/types/lib/store';

import {manifest} from '@/manifest';
import manifest from '@/manifest';

import {PluginRegistry} from '@/types/mattermost-webapp';

Expand Down
8 changes: 1 addition & 7 deletions webapp/src/manifest.test.tsx
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();
});
3 changes: 0 additions & 3 deletions webapp/src/manifest.ts

This file was deleted.