-
Notifications
You must be signed in to change notification settings - Fork 101
feat: magefile #180
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
base: master
Are you sure you want to change the base?
feat: magefile #180
Conversation
@fmartingr Nice work, I like the overall idea 👍 |
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.
A couple of things to note below, but I probably don't need to re-review so approving :)
@@ -4,15 +4,20 @@ on: | |||
- cron: "0 0 * * *" | |||
push: | |||
branches: | |||
- master | |||
- main |
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.
To confirm, is the demo
plugin's default branch changing from master
to main
? (I'm not sure it's worth doing this unless we were going to be consistent everywhere.)
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.
This is caused by mage taking care of the common files (aka. ci pipelines). The ci files are present here and the tooling updates it if there's a different with the file in the plugin repository.
jobs: | ||
plugin-ci: | ||
uses: mattermost/actions-workflows/.github/workflows/plugin-ci.yml@main | ||
uses: mattermost/actions-workflows/.github/workflows/plugin-ci.yml@mage |
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.
@mage
feels a bit odd -- why not just continue to rely on main
?
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.
CI needs to install mage
. The changes in https://github.com/mattermost/actions-workflows/commits/mage/ are required for CI to pass.
@fmartingr Is mattermost/actions-workflows#53 ready to get reviewed?
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.
FWIW: If we commit to the pattern from mattermost/mattermost-plugin-starter-template#217, we don't need the CI changes. Plugins can control the mage version themself.
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.
Yes, this workflow update handles having mage
available in the CI pipeline, the team told me that until this was ready to go the PR should remain blocked to prevent adding noise to the pipelines.
FWIW: If we commit to the pattern from mattermost/mattermost-plugin-starter-template#217, we don't need the CI changes. Plugins can control the mage version themself.
Unsure how this solves the issue completely if we avoid using make
altogether, since we would need mage
to call go tool
, unless we keep using make
as a wrapper.
@@ -6,18 +6,14 @@ linters-settings: | |||
gofmt: | |||
simplify: true | |||
goimports: | |||
local-prefixes: github.com/mattermost/mattermost-plugin-demo | |||
local-prefixes: github.com/mattermost/mattermost-starter-template |
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.
Is this an accidental copy-paste?
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.
Probably. or related to this.
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.
Neat! I wonder if we should port the changes from mattermost/mattermost-plugin-starter-template#217 and use go tool
to invoke mage
as that makes the CI pipeline easier.
Hello - I randomly found this change linked from my issue in Go repo and as a longtime Makefile hater and also mage user, I thought I would share some experience with it. After using it for a while, I migrated to goyek which I have found to be quite a bit easier to use. The primary reason of trying out a different tool was mage hasn't had a release for years. This means it still misses fixes from years ago and still has some open issues that I personally have run into in builds. While trying it out, some unexpected benefits I found were generally more reliability since goyek just relies on Go to compile without any code generation, and I could enable parallel dependencies, which I never did with mage, since goyek formats the output for them beautifully. I have packaged my "standard build" into a library for reference on what the definitions look like. While having spend a lot of thought on reproducible builds, I am just an internet rando so feel free to ignore completely :) But couldn't help but share just in case there is any interest in considering alternatives, and if there isn't, mage is still much better than make and a fine tool. |
Thank you for your comment, internet rando! I personally didn't knew about goyek. @lieut-data did you knew about this library when you did your preliminary work on the toolkit migration? I would be nice to check out all possible options before fully committing to one. |
Thank you for sharing, @anuraaga! No, @fmartingr, goyek hadn't been on my radar when this idea was first brewing. @anuraaga, I'm curious what the cost of converting would be? For example, are we ok to focus on "getting out of Makefiles" and solving the core issue of sharing build code, and evaluate goyek separately? Or is it more of a one-way door where we'll find a potential future adoption of goyek stymied by the Mage momentum? |
Thanks @lieut-data - in principle, I think most of the work of migrating scripts to Go is shared for either approach so in a code way, there would be a lot less churn if migrating from mage to goyek in the future, which I've verified from migrating a few such codebases. But looking through mattermost/mattermost@88c077dd22b1 I notice that it seems the idea is to provide a public API of build tasks for plugin authors, which is a great idea. I don't think a public API can be defined in a way that would work for either implementation so releasing that could potentially be a one-way door if it should be stable. If that is the goal, then just to add the note that while https://github.com/curioswitch/go-build/blob/main/standard.go#L249 So I have found the process of creating a whole shareable build framework much improved with goyek, while if it's just self-contained tasks within a single repo they are fairly similar. But I see what you mean by magefile momentum from going through that commit. It's hard for me to say that should be held up / significantly reworked, including dealing with issues that may come up on that side (one I predict is there isn't a native concept of namespace so it would be naming convention). Hopefully this is useful information and helps buy more into whatever decision is taken! |
Hey @anuraaga, we are going to have some discussions on how to move the plugin tooling forward and your input and details are really appreciated. Hopefully we will get to a decision soon. Thank you! |
Summary
Request for feedback.
Context: Mage is a replacement of
make
but using targets written in Go. More information: https://magefile.org/For folks added as reviewers in this PR, please give your feedback on this and the sibling PR in the monorepo. This is an
I'd like for people to review the ongoing effort of migrating the plugin tooling from
make
tomage
. High level details are here.This first iteration addresses the migration of some basic commands, refactoring duplicated logic into a main codebase (
dist/pluginctl
&dist/manifest
) and shows the overall feasibility of this effort.The list of commands migrated can be seen on the
Makefile
changes, mainly the ones to build and deploy the plugin into the server with some added bonuses. For more details into the commands please go into the accompanying server PR.The changes to the plugin are minimal and once migrated some of the files will be maintained by the new
pluginmage
package.The most critical path here is that we need to keep
server/public
updated in the plugins to a version. It will be breaking in some plugins with special logic or tied to an old mattermost dependency (not fully migrated to server/public). Also,public
not being versioned with a compatible server version may cause some headaches as well.Note: Please ignore some of the changes to the
.editorconfig
, CI workflows, etc. Those file are automatically updated when runningmage
and can be edited directly in the main package once all this goes live to have common ground for the plugin tooling.Please leave your questions, comments, feedback directly in this and the monorepo PR.
The target
Having the main plugin toolkit logic directly in go code is beneficial since we only need to upgrade the package dependency to update workflows, code, files, etc. Avoiding copy&pasting stuff all around plugins like we are doing today. This will prove useful for all 99% of plugins that will follow the same approach, with custom logic required for other (more complex) plugins that require special handling.
The good side here is that, since logic goes from Make targets to Go code, we can do anything directly avoiding some classic corner cases or just doing the same that make does: calling commands on the terminal. One example would be using
archive/tar
instead of calling thetar
command.Caveats
pluginmage
being insidepublic
(alongsidepluginapi
, since that made sense to me) means that everytime we updatepublic
we get the latest in the plugin ecosystem, for better or worse.mage
underneath, but we need to properly document and/or install mage automatically.Ticket Link