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.

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

Server dependency updates #969

Merged
merged 6 commits into from Jan 10, 2022
Merged

Server dependency updates #969

merged 6 commits into from Jan 10, 2022

Conversation

crspeller
Copy link
Member

Summary

Update dependencies!

@crspeller crspeller added the 2: Dev Review Requires review by a core committer label Jan 7, 2022
go.mod Outdated
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.


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/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.

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?

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.

@crspeller crspeller requested a review from cpoile January 10, 2022 17:29
@crspeller crspeller merged commit c2bc44a into master Jan 10, 2022
@crspeller crspeller deleted the server-dependancy-updates branch January 10, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants