-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add build info #598
Add build info #598
Conversation
plugin.json
Outdated
@@ -7,7 +7,7 @@ | |||
"release_notes_url": "https://github.com/mattermost/mattermost-plugin-github/releases/tag/v2.1.2", | |||
"icon_path": "assets/icon.svg", | |||
"version": "2.1.2", | |||
"min_server_version": "6.5.0", | |||
"min_server_version": "7.5.0", |
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.
On older versions, the plugin does fail to start due to mattermost/mattermost#21560.
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.
@hanzei How does that make it fail to start? Can we instead set the OwnerId
in this plugin? I don't like the situation that puts the plugin in, but it's backwards compatible and forwards compatible. The newer server will ignore any value passed in as expected, so it's not affected there.
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 instead set the
OwnerId
in this plugin?
Yes, we can do that, but I'm hesitant to tinker around with "manual" changes like that. The API already set the OwnderId
.
Do you mind the updated min_server_version
?
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.
Do you mind the updated
min_server_version
?
Yes, I would very much prefer keeping the server version lower. Having it this high will make it difficult to deliver bug fixes to (even slightly) older server versions. Any way around bumping the server version?
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.
Sure, added a workaround with 470cb0f.
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.
The packages that are commonly imported by plugins are model
and plugin
. Surely, we could release a separate modules file, but why?
This would also help with potential dependency bloat and dependency version management issues on the plugin project side.
I don't get that point. What bloat do you refer to? Currently, plugins import a single dependency: github.com/mattermost/mattermost-server/v6
. What other issues are you referring to?
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.
How much value are we gaining from these updates that require the newer server versions?
It's a necessity to continue using the pluginapi
package. By it's nature, it stays up to date relatively well with the MM server. And plugins just have to follow the imported server version.
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.
Ideally we would have an ESR process that spans out dot releases into older server versions, but we don't have that at the moment.
What is missing here in terms of process? If we found an bug as significant, that we would backport it to ESR, we can release of the latest release that is compatible with ESR.
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 missing here in terms of process? If we found an bug as significant, that we would backport it to ESR, we can release of the latest release that is compatible with ESR.
I meant having a separate ESR for plugin projects, but that would likely be too much overhead for the benefit it would bring. We do our best to deliver dot releases, but we don't necessarily take into account delivering those fixes to an older version of a plugin that has less minimum server version requirements.
I don't get that point. What bloat do you refer to? Currently, plugins import a single dependency:
github.com/mattermost/mattermost-server/v6
. What other issues are you referring to?
I meant the dependencies added to go.sum
. Do these contribute to the bundle size of the plugin?
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.
I meant the dependencies added to
go.sum
. Do these contribute to the bundle size of the plugin?
No, they don't. Only direct and indirect - i.e. transitive - dependencies increase the bundle size. E.g. github.com/mattermost/mattermost-server/v6/model
imports github.com/mattermost/mattermost-server/v6/shared/
so that package also gets compiled. github.com/francoispqt/gojay
is another example of a package that model
imports. Packages that are not direct or indirect imports do not contribute to the bundle site. github.com/mattermost/morph
gets imported by github.com/mattermost/mattermost-server/v6/store/sqlstore
and is part of go.sum
because the package github.com/mattermost/mattermost-server/v6/
includes it in it's go.mod
. But it does not contribute to the bundle size as it's not an import.
If one wants to dig deeper: nm
can be used to get the list of symbols from an object file, e.g. nm server/dist/plugin-linux-arm64
. That list contains all the symbols that contribute to the bundle size. If we want to improve the bundle size, we need to find imports that could be avoided if packaged in a different way. github.com/mattermost/mattermost-server/v6/model
imports github.com/mattermost/mattermost-server/v6/shared/markdown
only to use it in RewriteImageURLs
. Maybe RewriteImageURLs
could be moved in github.com/mattermost/mattermost-server/v6/shared/markdown
and the import could be avoided.
But these are small optimisation. In general, the compiler is smarter than the programmer 😉
Codecov ReportBase: 15.67% // Head: 15.63% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #598 +/- ##
==========================================
- Coverage 15.67% 15.63% -0.05%
==========================================
Files 15 15
Lines 5231 5245 +14
==========================================
Hits 820 820
- Misses 4368 4382 +14
Partials 43 43
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
LGTM!
@mickmister This PR is ready for another review |
return &model.CommandResponse{}, nil | ||
} | ||
|
||
if action == "about" { |
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.
We usually name this command info
like /github info
. I'm thinking we should be consistent and continue to use this pattern cc @levb
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.
I went back and forth and ended with about
as that is a common term used in software to show build information.
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.
Since info
is already used in other plugin projects, and has been used as a tool with the support team to gather information plugin installations, 2/5 I'm thinking we should use info
here
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.
has been used as a tool with the support team to gather information plugin installations,
The only plugin used in production that uses an info
command is Jira, right? Even that command works slightly differently. I would like for the about
command to not include any confidential information, so it can be used for debugging without manual cleanup.
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.
@mickmister What do you think about my comment?
/update-branch |
Summary
Make use of mattermost/mattermost-plugin-api#121.
Ticket Link
None