Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Server dependency updates #969

Merged
merged 6 commits into from
Jan 10, 2022
Merged

Server dependency updates #969

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4a43c8
Update server dependancy.
crspeller Jan 7, 2022
9759070
Do 1.16 because build image is stuck there.
crspeller Jan 7, 2022
32223f2
Go mod tidy.
crspeller Jan 7, 2022
b4252f5
Fix plugin api.
crspeller Jan 7, 2022
4ba03e6
Merge branch 'master' into server-dependancy-updates
crspeller Jan 7, 2022
6386e8d
Change to v0.7.0 for convinance. (value is ignored by go mod)
crspeller Jan 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 14 additions & 11 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,31 @@
module github.com/mattermost/mattermost-plugin-playbooks

go 1.14
go 1.16

replace github.com/mattermost/mattermost-plugin-playbooks/client => ./client
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why before the require instead of after? (Seems slightly weird to start reading replace directives when I haven't even encountered what they replace.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what go mod tidy did. 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like it because the require stuff is mostly meaningless. The replace stuff is more important.


replace github.com/HdrHistogram/hdrhistogram-go => github.com/codahale/hdrhistogram v1.1.2

replace github.com/golang/mock => github.com/golang/mock v1.4.4

replace github.com/mattermost/mattermost-server/v6 => github.com/mattermost/mattermost-server/v6 v6.0.0-20220107143254-93a9c6c2931f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, why do we need this separate from below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes updates easier by locking the server version since we need this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an opportunity to document this somehow? At first glance, it feels like a "broken" usage of the replace directive for something that's supposed to be in the require block. Is it an oddity related to how we version the server?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an oddity because there is no released server version. If you try to install anything else it will redo this and grab the most recent server version (which would actually be a downgrade). This just helpfully pins the version so we don't have to worry about it downgrading randomly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not sure I understand -- why would the hashed version dependency (that we've always used) be downgraded?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do a go get and add another module without the replace directive it's going to do it's minimal version selection and likely try to downgrade mattermost-server. With the replace we force it to stay there until we explicitly change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if it works it works, but we haven't had a problem with it before, have we?


replace github.com/mattermost/mattermost-plugin-api => github.com/mattermost/mattermost-plugin-api v0.0.22-0.20211207232216-3faec618d311

require (
github.com/BurntSushi/toml v0.4.1 // indirect
github.com/Masterminds/squirrel v1.5.1
github.com/Masterminds/squirrel v1.5.2
github.com/blang/semver v3.5.1+incompatible
github.com/go-sql-driver/mysql v1.6.0
github.com/golang/mock v1.6.0
github.com/gorilla/mux v1.8.0
github.com/jmoiron/sqlx v1.3.4
github.com/lib/pq v1.10.4
github.com/mattermost/mattermost-plugin-api v0.0.22-0.20211207232216-3faec618d311
github.com/mattermost/mattermost-plugin-playbooks/client v0.6.0
github.com/mattermost/mattermost-plugin-playbooks/client v0.0.0-00010101000000-000000000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is updated in #968 :P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? The replace line make this line meaningless. Might as well be the 000000 thing right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo: because you can comment the replace directive out and run the tests to make sure the actual published module is correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpoile But the replace directive is part of the module? There isn't some other version of the module as far as I am aware?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "the replace directive is part of the module", sorry. There is version v0.7.0 which we just published... Unless you're talking about something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synced offline and figured out what each other where saying. This line is totally ignored by go mod when the replace is active. But you can test mismatched versions when you comment out a replace and fill in a version here.

github.com/mattermost/mattermost-server/v6 v6.0.0-20211207185652-92e80bd4ed31
github.com/pkg/errors v0.9.1
github.com/rudderlabs/analytics-go v3.3.1+incompatible
github.com/rudderlabs/analytics-go v3.3.2+incompatible
github.com/sirupsen/logrus v1.8.1
github.com/stretchr/testify v1.7.0
github.com/writeas/go-strip-markdown v2.0.1+incompatible
)

replace github.com/mattermost/mattermost-plugin-playbooks/client => ./client

replace github.com/HdrHistogram/hdrhistogram-go => github.com/codahale/hdrhistogram v1.1.2

replace github.com/golang/mock v1.6.0 => github.com/golang/mock v1.4.4