[MM-17889] Implement validation of plugin API version comments #11941

pbitty · 2019-08-23T03:44:59Z

This is to resolve issue #11910. All feedback is welcome.

pbitty · 2019-08-23T03:49:07Z

I am wondering about where to invoke this check from. Should it be part of the make test target, or perhaps added to scripts/test.sh?

Another thought I had was that this could be put into a unit test so that it simply runs as part of go test. What are your thoughts on that idea?

lieut-data

This is terrific work, @pbitty! And welcome to the community!

I've left some thoughts below, with one major piece of feedback to converge on fmt/lint output semantics. I recently stumbled across the ability to write custom analyzers for go vet, and think this script could make a worthy candidate (eliminating the need to do the manual AST processing). It's not necessary to go down the go vet path for this pull request, but I do think we should aim for similar output semantics.

plugin/checker/main.go

levb

Great job, @pbitty! I agree and have nothing to add to the feedback from @lieut-data. Please re-add me as a reviewer when it's implemented.

I also agree that adding it as a go vet stage is a great idea, so probably best as a standalone executable, invoked right next to go vet in the Makefile, for now?

This reverts commit d4f893a.

pbitty · 2019-08-27T03:40:50Z

@lieut-data @levb , thanks for the feedback and your kind words. I believe I have addressed your comments.

I took a look at the custom analyzers article. I would be happy submit a go vet implementation after this one is merged. Or is it better that another issue is created to tackle it later on?

lieut-data

This... is... AMAZING. Thank you @pbitty! I love open-source :)

pbitty · 2019-08-31T01:55:13Z

@levb @lieut-data, you are very welcome!

I took a look at the approach for implementing it as an "Analyzer" using go/analysis (the same package used by go vet). It seems it would make the code simpler and keep the output consistent with the rest of the Go tools.

The only potential catch I found is that the go/analysis package doesn't give you a way to point it at a specific package (ie: github.com/mattermost/mattermost-server/plugin). It simply provides a singlechecker.Main() method, which handles CLI args and expects a package to be passed in, similar to go vet.

This means the checker code would have no knowledge of the plugin package at all, it would have to be passed in as a CLI arg, eg:

go run plugin/checker/main.go github.com/mattermost/mattermost-server/plugin

To me this sounds reasonable and still worth implementing. What do you think?

lieut-data · 2019-08-31T02:31:30Z

@pbitty: I agree with your analysis and recommendation. Are you still interested in running with this change? Would it make sense just to do it in this PR?

pbitty · 2019-08-31T23:38:38Z

@lieut-data I've just pushed the change. There are a couple of little hacks I had to narrow it down to the file set that contains the API interface. This approach does eliminate the code for loading the package and printing errors. What do you think?

Looking at #11912, some of this code can probably be refactored to help there.

I've also gone ahead and vendored in the new dependencies, following the example of other PRs.

pbitty · 2019-08-31T23:42:23Z

The tests seem to be failing because of a package that was removed by go mod vendor. I will look into this as soon as possible.

pbitty · 2019-09-06T03:38:49Z

@lieut-data, I took the liberty to go with my suggestion above. Let me know what you think and I can change it as needed.

levb · 2019-09-06T18:46:57Z

👍 thanks for the tests! Looks like it's now failing on

+ make check-style BUILD_NUMBER=PR-11941-21
Running GOVET
go get golang.org/x/tools/go/analysis/passes/shadow/cmd/shadow
go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
	ignoring go.mod;
...

and then failing to find packages. I am not up to speed as to go mod and vendor/ status in mattermost-server, @lieut-data perhaps can help look into this?

Properly reporting a non-existent package seems to require some very specific error parsing. The `Load` method already returns a number of import errors because it cannot find all of the dependencies of the `plugin` package. This makes it difficult to discern between a "cannot find package" error and the ignorable import errors that are regularly returned. Setting up the `Load` method to find all dependencies and load the package without import errors is not required for our use-case. The ability to report a build-stage error that is very rare and easy enough to troubleshoot, does not seem to warrant the added complexity that it would require.

pbitty · 2019-09-09T14:28:39Z

@lieut-data , the tests have been consistently failing at this test:

--- FAIL: TestConfigSet (3.20s)
    --- PASS: TestConfigSet/Error_when_no_arguments_are_given (0.10s)
    --- PASS: TestConfigSet/Error_when_only_one_argument_is_given (0.10s)
    --- PASS: TestConfigSet/Error_when_the_wrong_key_is_set (0.39s)
    --- FAIL: TestConfigSet/Error_when_the_wrong_value_is_set (0.48s)
        config_test.go:141: 
            	Error Trace:	config_test.go:141
            	Error:      	"=== RUN   TestExecCommand
            	            	EmailSettings.ConnectionSecurity: ""
            	            	--- PASS: TestExecCommand (0.13s)
            	            	PASS
            	            	coverage: 1.6% of statements in github.com/mattermost/mattermost-server/api4, github.com/mattermost/mattermost-server/app, github.com/mattermost/mattermost-server/cmd/mattermost, github.com/mattermost/mattermost-server/cmd/mattermost/commands, github.com/mattermost/mattermost-server/cmd/platform, github.com/mattermost/mattermost-server/config, github.com/mattermost/mattermost-server/config/config_generator, github.com/mattermost/mattermost-server/config/config_generator/generator, github.com/mattermost/mattermost-server/einterfaces, github.com/mattermost/mattermost-server/einterfaces/jobs, github.com/mattermost/mattermost-server/einterfaces/mocks, github.com/mattermost/mattermost-server/imports, github.com/mattermost/mattermost-server/jobs, github.com/mattermost/mattermost-server/jobs/interfaces, github.com/mattermost/mattermost-server/manualtesting, github.com/mattermost/mattermost-server/migrations, github.com/mattermost/mattermost-server/mlog, github.com/mattermost/mattermost-server/mlog/human, github.com/mattermost/mattermost-server/model, github.com/mattermost/mattermost-server/model/gitlab, github.com/mattermost/mattermost-server/plugin, github.com/mattermost/mattermost-server/plugin/checker, github.com/mattermost/mattermost-server/plugin/checker/test/invalid, github.com/mattermost/mattermost-server/plugin/checker/test/missing, github.com/mattermost/mattermost-server/plugin/checker/test/valid, github.com/mattermost/mattermost-server/plugin/interface_generator, github.com/mattermost/mattermost-server/plugin/plugintest, github.com/mattermost/mattermost-server/plugin/plugintest/mock, github.com/mattermost/mattermost-server/plugin/scheduler, github.com/mattermost/mattermost-server/services/configservice, github.com/mattermost/mattermost-server/services/filesstore, github.com/mattermost/mattermost-server/services/filesstore/mocks, github.com/mattermost/mattermost-server/services/httpservice, github.com/mattermost/mattermost-server/services/imageproxy, github.com/mattermost/mattermost-server/services/mailservice, github.com/mattermost/mattermost-server/services/mfa, github.com/mattermost/mattermost-server/services/timezones, github.com/mattermost/mattermost-server/store, github.com/mattermost/mattermost-server/store/layer_generators, github.com/mattermost/mattermost-server/store/localcachelayer, github.com/mattermost/mattermost-server/store/sqlstore, github.com/mattermost/mattermost-server/store/storetest, github.com/mattermost/mattermost-server/store/storetest/mocks, github.com/mattermost/mattermost-server/testlib, github.com/mattermost/mattermost-server/utils, github.com/mattermost/mattermost-server/utils/fileutils, github.com/mattermost/mattermost-server/utils/imgutils, github.com/mattermost/mattermost-server/utils/jsonutils, github.com/mattermost/mattermost-server/utils/markdown, github.com/mattermost/mattermost-server/utils/testutils, github.com/mattermost/mattermost-server/web, github.com/mattermost/mattermost-server/wsapi, github.com/mattermost/mattermost-server/enterprise/account_migration, github.com/mattermost/mattermost-server/enterprise/cluster, github.com/mattermost/mattermost-server/enterprise/compliance, github.com/mattermost/mattermost-server/enterprise/data_retention, github.com/mattermost/mattermost-server/enterprise/elasticsearch, github.com/mattermost/mattermost-server/enterprise/imports, github.com/mattermost/mattermost-server/enterprise/ldap, github.com/mattermost/mattermost-server/enterprise/message_export, github.com/mattermost/mattermost-server/enterprise/message_export/actiance_export, github.com/mattermost/mattermost-server/enterprise/message_export/common_export, github.com/mattermost/mattermost-server/enterprise/message_export/csv_export, github.com/mattermost/mattermost-server/enterprise/message_export/global_relay_export, github.com/mattermost/mattermost-server/enterprise/metrics, github.com/mattermost/mattermost-server/enterprise/oauth/google, github.com/mattermost/mattermost-server/enterprise/oauth/office365, github.com/mattermost/mattermost-server/enterprise/saml" should not contain "invalid"
            	Test:       	TestConfigSet/Error_when_the_wrong_value_is_set
    --- FAIL: TestConfigSet/Error_when_the_wrong_locale_is_set (0.72s)
        config_test.go:148: 
            	Error Trace:	config_test.go:148
            	Error:      	"=== RUN   TestExecCommand
            	            	LocalizationSettings.DefaultServerLocale: "es"
            	            	--- PASS: TestExecCommand (0.13s)
            	            	PASS
            	            	coverage: 1.6% of statements in github.com/mattermost/mattermost-server/api4, github.com/mattermost/mattermost-server/app, github.com/mattermost/mattermost-server/cmd/mattermost, github.com/mattermost/mattermost-server/cmd/mattermost/commands, github.com/mattermost/mattermost-server/cmd/platform, github.com/mattermost/mattermost-server/config, github.com/mattermost/mattermost-server/config/config_generator, github.com/mattermost/mattermost-server/config/config_generator/generator, github.com/mattermost/mattermost-server/einterfaces, github.com/mattermost/mattermost-server/einterfaces/jobs, github.com/mattermost/mattermost-server/einterfaces/mocks, github.com/mattermost/mattermost-server/imports, github.com/mattermost/mattermost-server/jobs, github.com/mattermost/mattermost-server/jobs/interfaces, github.com/mattermost/mattermost-server/manualtesting, github.com/mattermost/mattermost-server/migrations, github.com/mattermost/mattermost-server/mlog, github.com/mattermost/mattermost-server/mlog/human, github.com/mattermost/mattermost-server/model, github.com/mattermost/mattermost-server/model/gitlab, github.com/mattermost/mattermost-server/plugin, github.com/mattermost/mattermost-server/plugin/checker, github.com/mattermost/mattermost-server/plugin/checker/test/invalid, github.com/mattermost/mattermost-server/plugin/checker/test/missing, github.com/mattermost/mattermost-server/plugin/checker/test/valid, github.com/mattermost/mattermost-server/plugin/interface_generator, github.com/mattermost/mattermost-server/plugin/plugintest, github.com/mattermost/mattermost-server/plugin/plugintest/mock, github.com/mattermost/mattermost-server/plugin/scheduler, github.com/mattermost/mattermost-server/services/configservice, github.com/mattermost/mattermost-server/services/filesstore, github.com/mattermost/mattermost-server/services/filesstore/mocks, github.com/mattermost/mattermost-server/services/httpservice, github.com/mattermost/mattermost-server/services/imageproxy, github.com/mattermost/mattermost-server/services/mailservice, github.com/mattermost/mattermost-server/services/mfa, github.com/mattermost/mattermost-server/services/timezones, github.com/mattermost/mattermost-server/store, github.com/mattermost/mattermost-server/store/layer_generators, github.com/mattermost/mattermost-server/store/localcachelayer, github.com/mattermost/mattermost-server/store/sqlstore, github.com/mattermost/mattermost-server/store/storetest, github.com/mattermost/mattermost-server/store/storetest/mocks, github.com/mattermost/mattermost-server/testlib, github.com/mattermost/mattermost-server/utils, github.com/mattermost/mattermost-server/utils/fileutils, github.com/mattermost/mattermost-server/utils/imgutils, github.com/mattermost/mattermost-server/utils/jsonutils, github.com/mattermost/mattermost-server/utils/markdown, github.com/mattermost/mattermost-server/utils/testutils, github.com/mattermost/mattermost-server/web, github.com/mattermost/mattermost-server/wsapi, github.com/mattermost/mattermost-server/enterprise/account_migration, github.com/mattermost/mattermost-server/enterprise/cluster, github.com/mattermost/mattermost-server/enterprise/compliance, github.com/mattermost/mattermost-server/enterprise/data_retention, github.com/mattermost/mattermost-server/enterprise/elasticsearch, github.com/mattermost/mattermost-server/enterprise/imports, github.com/mattermost/mattermost-server/enterprise/ldap, github.com/mattermost/mattermost-server/enterprise/message_export, github.com/mattermost/mattermost-server/enterprise/message_export/actiance_export, github.com/mattermost/mattermost-server/enterprise/message_export/common_export, github.com/mattermost/mattermost-server/enterprise/message_export/csv_export, github.com/mattermost/mattermost-server/enterprise/message_export/global_relay_export, github.com/mattermost/mattermost-server/enterprise/metrics, github.com/mattermost/mattermost-server/enterprise/oauth/google, github.com/mattermost/mattermost-server/enterprise/oauth/office365, github.com/mattermost/mattermost-server/enterprise/saml" should not contain "invalid"
            	Test:       	TestConfigSet/Error_when_the_wrong_locale_is_set

The test seems to be failing because I introduced the checker/test/invalid package, which contains the word invalid. I will take a deeper look either tonight or tomorrow night (ET). In the meantime, if you have any suggestions, please let me know.

… GH-11910_enforce_version_comments

pbitty · 2019-09-11T04:24:38Z

@lieut-data ,

I did a bit more digging and discovered that the output of RunCommand in the tests includes go test output, and when coverage reporting is enabled, it prints all the package names. I added a package called invalid, which ends up being caught by the tests that assert on the absence of the word invalid.

You can reproduce it without running all the test with this command:

make TE_PACKAGES="github.com/mattermost/mattermost-server/cmd/mattermost/commands github.com/mattermost/mattermost-server/plugin/checker/test/invalid" TESTS=TestConfigSet test-server

Pending a deeper fix, I think simply changing the key to something else should be enough for this PR. Let me know if you agree.

lieut-data · 2019-09-11T12:13:42Z

@pbitty, sorry for the delay in responding -- I'm currently out-of-office running a team meetup and have been slow to respond.

Winding the stack backwards, I think renaming the package to workaround the clash is fine. Thanks for digging into this!

On the go/analysis front, I'm 0/5, as both of your solutions seem really good. If we can unit test the approach you have now, that's great! I'm curious if the Go team had a plan for enabling unit testing using the go/analysis approach, but we can iterate on that independently.

Let me know if you think we're good, and I'll go ahead and merge this :)

pbitty · 2019-09-11T13:48:53Z

@lieut-data , no problem at all. I think it's good to merge.

Before your reply, I had already renamed the key in the TestConfigSet test instead of the package name. If you're okay with that, we're good to merge. Otherwise, I can tweak it still.

lieut-data · 2019-09-11T18:49:49Z

Excellent! Looks good from my perspective -- @prapti, when you have a chance, can you give us guidance on whether we test before or after merging this feature?

… GH-11910_enforce_version_comments

prapti

This PR will be tested post merge.
Approving.
Removing QA Review label

hanzei · 2019-09-17T06:04:01Z

Thank you very much for this awesome PR @pbitty 🕺

mattermod · 2019-10-10T20:50:57Z

Test server destroyed

pbitty added 2 commits August 22, 2019 23:41

Implement validation of plugin API version comments

6f3705a

MM-17889

Add missing minimum server version comments to API interface methods

6c16b87

MM-17889

Add license header

fc1bace

hanzei requested a review from lieut-data August 23, 2019 21:30

hanzei added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Aug 23, 2019

hanzei requested a review from prapti August 23, 2019 21:30

lieut-data requested changes Aug 26, 2019

View reviewed changes

lieut-data requested a review from levb August 26, 2019 14:37

levb reviewed Aug 26, 2019

View reviewed changes

pbitty added 8 commits August 26, 2019 21:54

Simplify console output

109d3d3

Inline single-use function

e67175e

Simplify loading of package files and output package name on error

5efc4e7

Simplify error output and exit code generation

7e44d8c

Run plugin API comments checker as part of govet target

9431f53

Remove some minimum version comments to trigger a build failure

d4f893a

Revert "Remove some minimum version comments to trigger a build failure"

e4a7628

This reverts commit d4f893a.

Remove superfluous newline in console output

66960f2

levb approved these changes Aug 27, 2019

View reviewed changes

lindalumitchell added this to the v5.16.0 milestone Aug 27, 2019

lieut-data approved these changes Aug 27, 2019

View reviewed changes

lieut-data removed the 2: Dev Review Requires review by a developer label Aug 27, 2019

pbitty added 2 commits August 31, 2019 19:17

Use go/analysis package for checking plugin comments

a7ceb3e

Vendor golang.org/x/tools/analysis and dependencies

bd619e7

Vendor golang.org/x/tools/go/packages and dependencies

b230f28

pbitty changed the title ~~GH-11910: Implement validation of plugin API version comments~~ [MM-17889] Implement validation of plugin API version comments Sep 7, 2019

pbitty added 3 commits September 6, 2019 21:47

Add license headers

25fffbd

Fix expected error in test case

180666e

Merge branch 'master' into GH-11910_enforce_version_comments

5c11075

pbitty added 2 commits September 10, 2019 13:45

Merge branch 'master' of github.com:mattermost/mattermost-server into…

ae80061

… GH-11910_enforce_version_comments

Work around config test error string matching

28a6a51

Merge branch 'master' of github.com:mattermost/mattermost-server into…

ad83884

… GH-11910_enforce_version_comments

prapti approved these changes Sep 16, 2019

View reviewed changes

prapti removed the 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review label Sep 16, 2019

hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Sep 17, 2019

hanzei self-assigned this Sep 17, 2019

hanzei merged commit 5b79fc4 into mattermost:master Sep 17, 2019

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Sep 17, 2019

hanzei mentioned this pull request Oct 7, 2019

Enforce minimum server version comment in plugin/api.go. #11910

Closed

prapti added Setup Cloud Test Server Setup an on-prem test server and removed Setup Cloud Test Server Setup an on-prem test server labels Oct 10, 2019

hanzei removed their assignment Sep 25, 2020

pbitty deleted the GH-11910_enforce_version_comments branch February 7, 2024 03:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-17889] Implement validation of plugin API version comments #11941

[MM-17889] Implement validation of plugin API version comments #11941

pbitty commented Aug 23, 2019 •

edited

Loading

pbitty commented Aug 23, 2019

lieut-data left a comment

levb left a comment

pbitty commented Aug 27, 2019

lieut-data left a comment

pbitty commented Aug 31, 2019

lieut-data commented Aug 31, 2019

pbitty commented Aug 31, 2019

pbitty commented Aug 31, 2019

pbitty commented Sep 6, 2019

levb commented Sep 6, 2019

pbitty commented Sep 9, 2019

pbitty commented Sep 11, 2019

lieut-data commented Sep 11, 2019

pbitty commented Sep 11, 2019

lieut-data commented Sep 11, 2019

prapti left a comment

hanzei commented Sep 17, 2019 •

edited

Loading

mattermod commented Oct 10, 2019

[MM-17889] Implement validation of plugin API version comments #11941

[MM-17889] Implement validation of plugin API version comments #11941

Conversation

pbitty commented Aug 23, 2019 • edited Loading

pbitty commented Aug 23, 2019

lieut-data left a comment

Choose a reason for hiding this comment

levb left a comment

Choose a reason for hiding this comment

pbitty commented Aug 27, 2019

lieut-data left a comment

Choose a reason for hiding this comment

pbitty commented Aug 31, 2019

lieut-data commented Aug 31, 2019

pbitty commented Aug 31, 2019

pbitty commented Aug 31, 2019

pbitty commented Sep 6, 2019

levb commented Sep 6, 2019

pbitty commented Sep 9, 2019

pbitty commented Sep 11, 2019

lieut-data commented Sep 11, 2019

pbitty commented Sep 11, 2019

lieut-data commented Sep 11, 2019

prapti left a comment

Choose a reason for hiding this comment

hanzei commented Sep 17, 2019 • edited Loading

mattermod commented Oct 10, 2019

pbitty commented Aug 23, 2019 •

edited

Loading

hanzei commented Sep 17, 2019 •

edited

Loading