MM-32148 - Generate version metadata from tags by cpoile · Pull Request #433 · mattermost/mattermost-plugin-playbooks

cpoile · 2021-01-21T19:28:39Z

Summary

If the current commit has a tag, use that as the version (without the prefix 'v'). Otherwise, use the latest tag and add the short hash, eg. "1.2.0+9fcd0b92".
Q: should we also update the release_notes_url in plugin.json?

Ticket Link

https://mattermost.atlassian.net/browse/MM-32148

cpoile · 2021-01-21T20:08:59Z

Doing some experimentation with e2e tests, will request reviews when that's all over with

lieut-data

Nice! A few comments below.

build/setup.mk

lieut-data · 2021-01-22T16:54:12Z

plugin.json

    "release_notes_url": "https://github.com/mattermost/mattermost-plugin-incident-management/releases/tag/v1.2.0",
    "icon_path": "assets/incident_plugin_icon.svg",
-    "version": "1.2.0",
+    "version": "1.2.0+9fcd0b92",


Propose we remove the version attribute from the checked-in file otherwise we'll always have a permanently old version checked in.

I tried to do that, but couldn't think of a way to do it--bin/manifest will output the whole plugin.json, including the version field

Can we just manually edit the file in this PR to remove the version attribute? I guess then the manifest will always leave a modified plugin.json in the repo... which would be annoying. Can we write the output to a temporary plugin.json (i.e. the one that's being put into the bundle path)?

Yep, we can write the plugin.json anywhere, and change the bundle instructions. Will do that.

There, the plugin.json is back in the root, but without the version field. The version field is generated and put into the build directory, and the make bundle copies that generated one to the dist folder. Looks good now, thanks!

lieut-data · 2021-01-22T16:55:14Z

server/manifest.go

  "release_notes_url": "https://github.com/mattermost/mattermost-plugin-incident-management/releases/tag/v1.2.0",
  "icon_path": "assets/incident_plugin_icon.svg",
-  "version": "1.2.0",
+  "version": "1.2.0+9fcd0b92",


Should we remove these files from the repo, and let them get generated only at build time? (Do we also .gitignore them?)

I was thinking of that, but then we would have to have the "base" plugin.json somewhere, so that we can read it in. I was thinking of putting it in the build directory. What do you think?

I made that change so we can see what it looks like. It's not perfect though, because we still update the manifest files with the version info, and I can't think of a way to not have to check that in. Maybe ignore the manifest files as well since they're generated? Hmm, let's see if that will work.

There, I think that works. Now the three generated files are not checked in. Only thing that feels a little "wrong" is putting the base plugin.json file in the build directory. Open to alternatives there. But if we keep it in the root directory it will be updated and we will start to check in old version numbers.

Oops, commented above before I read this thread. Yeah, wondering if we can keep the root one the base one, and change manifest to write to the temporary one in the bundle directory at build time?

As a compromise we're writing into the build directory, because we rm the dist directory before copying the manifest, and I don't want to mess around with that. :)

build/setup.mk

cpoile · 2021-01-22T21:12:00Z

@lieut-data @crspeller ready for re-review, then we can start testing the cloud build process. :)

lieut-data

Thanks, @cpoile! One comment below about build/plugin.json.

lieut-data · 2021-01-25T15:39:22Z

Makefile

 CURL ?= $(shell command -v curl 2> /dev/null)
 MM_DEBUG ?=
-MANIFEST_FILE ?= plugin.json
+MANIFEST_FILE ?= build/plugin.json


Hmm, this doesn't "smell right" to me -- at least from the context of reading the Makefile, MANIFEST_FILE is about defining the source of truth. This file doesn't exist yet: I'd love to see whatever temporary path we use be an implementation detail vs. something that's configured like this.

Maybe we just make pass the desired output path to bin/manifest and invoke it manually within the bundle target? Or, actually, "applying" plugin.json only needs to happen within the bundle target, so maybe it's just a different bin/manifest apply_version command or some such.

Yep, that does work better, nice idea. I added a manifest dist command to write the plugin.json to the dist directory. Thanks!

lieut-data

🎉

crspeller

Awesome!

crspeller · 2021-01-25T16:13:43Z

plugin.json

            "windows-amd64": "server/dist/plugin-windows-amd64.exe"
-        }
+        },
+        "executable": ""


Why did this come back?

Looks like it was generated by marshalling the manifest struct. I can not check it in if it's causing confusion?

@cpoile Nope its fine.

use HEAD tag, or else latest tag + short hash

9090625

cpoile added the 2: Dev Review Requires review by a core committer label Jan 21, 2021

cpoile requested review from crspeller and lieut-data January 21, 2021 19:28

testing e2e with different mattermost docker images

7a871bf

cpoile removed request for crspeller and lieut-data January 21, 2021 20:09

cpoile removed the 2: Dev Review Requires review by a core committer label Jan 21, 2021

cpoile added 2 commits January 21, 2021 17:23

turn off admin notices

64d0edd

e2e master only on branches, 531 for deploy-release

8118ebd

cpoile force-pushed the MM-32148-generate-version-metadata branch from 22e5420 to 8118ebd Compare January 21, 2021 23:33

no need to run 530 also, only need to run the latest release

f985294

cpoile requested review from crspeller and lieut-data January 21, 2021 23:54

cpoile added the 2: Dev Review Requires review by a core committer label Jan 21, 2021

Revert e2e testing fixes

3352b70

lieut-data requested changes Jan 22, 2021

View reviewed changes

cpoile added 5 commits January 22, 2021 13:01

Merge branch 'master' into MM-32148-generate-version-metadata

2940de1

move plugin.json to build directory

3b479b6

remove generated manifest files

4d7a25b

apply the manifest before running the tests

4124d34

apply the manifest before running check-style

51f683d

cpoile requested a review from lieut-data January 22, 2021 18:53

cpoile added 2 commits January 22, 2021 14:05

keep plugin.json at root without version #

4fe6863

Merge branch 'master' into MM-32148-generate-version-metadata

18d049f

lieut-data requested changes Jan 25, 2021

View reviewed changes

write plugin.json into dist directory directly

384ac9b

cpoile requested a review from lieut-data January 25, 2021 16:08

lieut-data approved these changes Jan 25, 2021

View reviewed changes

crspeller approved these changes Jan 25, 2021

View reviewed changes

cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jan 25, 2021

cpoile merged commit aea15de into master Jan 25, 2021

cpoile deleted the MM-32148-generate-version-metadata branch January 25, 2021 16:33

lieut-data mentioned this pull request Dec 12, 2023

Sync with playbooks: install-go-tools, gotestsum, and dynamic versions mattermost/mattermost-plugin-starter-template#192

Merged

Conversation

cpoile commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket Link

Uh oh!

cpoile commented Jan 21, 2021

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpoile Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpoile commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lieut-data left a comment

Choose a reason for hiding this comment

Uh oh!

crspeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpoile commented Jan 21, 2021 •

edited

Loading

cpoile Jan 22, 2021 •

edited

Loading

cpoile commented Jan 22, 2021 •

edited

Loading