-
Notifications
You must be signed in to change notification settings - Fork 8
feat: upgrade to magefile #136
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
Conversation
@wiggin77 @lieut-data Requested you review to check on this first iteration of the effort. Failing e2e tests are due to #138. |
/update-branch |
# magefile included assets | ||
/.golangci.yml | ||
/.editorconfig | ||
#/.github/workflows/ci.yml # Has to be committed to the repo for github to use it |
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.
Can we not just delete this line?
Shouldn't the mage code be in its own repo so it can be shared across plugins? |
The idea is to have this approved so we can move it to the public package in the monorepo, so the pluginapi and pluginmage live togeher. |
If we plan to move this forward long term I also need confirmation to press on mattermost/actions-workflows#53 |
To continue the discussion here, folks have told me that this may be easier to review if the plugin package and the changes to this repo were split into two. My idea was to have the I want to unblock this so we can get it reviewed, so please share your feedback about this here so we can move this along. cc: @wiggin77 @lieut-data @hanzei @davidkrauser @BenCookie95 |
I'd like to propose that we start by putting this in the Then, once we have momentum behind this, we can consider breaking it out into a separate repo if the pain of upgrading all of Thoughts @fmartingr? |
I'm fine with this being in the mono-repo. There is some benefit to that in keeping model version in sync. My only concern with it being in the "public" area is the downward compatibility guarantees that package has. This needs to be imported by all supported plugin repos, but I don't want to hand-cuff ourselves into not being able to iterate on it. |
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.
Incredibly excited with the direction this is heading! A few comments, but let's keep going :)
return fmt.Errorf("go is not available: see https://golang.org/doc/install. %s", buildErrorToDisplay(ErrInitGo)) | ||
} | ||
|
||
// Check if tar is GNU tar |
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.
Future: use https://pkg.go.dev/archive/tar and avoid this dep altogether.
underline = "\x1b[4m" | ||
reset = "\x1b[0m" | ||
|
||
// Colors for different log levels |
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.
Can we use logrus and outsource this logic instead?
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.
My idea was to use only builtins as much as possible, so we only depend on mage and the model package and prevent conflicts on plugins that use any other libraries. This customization is there for coloring and to easily spot namespace/target combination, which we would need to do for logrus as well.
I think the best spot for this is the public package unless we have strong evidence otherwise. Putting it in a separate repository may give us more freedom but we would end up in dependency problems either way, since I think we can land in two areas:
|
Summary
server
,proceesor
,webapp
,webapp/node_modules
,deploy
,dist
,bundle
,watch
Makefile
now callsmage
on the migrated targetsmage pluginctl:command
and it's available to other targets as well.Improvements
magefiles
, there's an example in this plugin with the processor.pluginmage
All logic is bundled to this package which is the one we can move to the public package in the mattermost server for distribution.
Check the included README.
Requires mattermost/actions-workflows#53