Skip to content

Update go dependencies #2008

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

Merged
merged 6 commits into from
Apr 29, 2019
Merged

Update go dependencies #2008

merged 6 commits into from
Apr 29, 2019

Conversation

jim
Copy link
Contributor

@jim jim commented Apr 12, 2019

Description

It seems that in the switch from dep to go mod a bunch of our dependencies were downgraded, probably due to how go mod uses the minimum version that will satisfy requirements for indirect dependencies.

This PR updates most of our dependencies to the latest version.

Reviewer Notes

You can run the script to update dependencies locally like so:

$ make go_deps_update

I noticed that go vet sometimes alters go.mod. I'm not sure why.

Code Review Verification Steps

  • Request review from a member of a different team.

References

Copy link
Contributor

@pjdufour-truss pjdufour-truss left a comment

Choose a reason for hiding this comment

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

Since we don't have a tagged release on github.com/trussworks/pdfcpu for our afero work, you need to run the following to tag the latest commit on the afero branch.

go get github.com/trussworks/pdfcpu@afero

## Update a specific dependency

```console
$ go get -u github.com/pkg/errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to also have an example of getting the dependency @master, which I think is the right syntax. I think a lot of our deps we want at the master version.

Copy link
Contributor

@chrisgilmerproj chrisgilmerproj left a comment

Choose a reason for hiding this comment

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

I'd love one more thing added to the README. But I'm not going to block. Thanks for doing this.

@jim jim added the roci label Apr 15, 2019
@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Apr 15, 2019

This PR is reverts cobra, viper, and pflag. Since we want master branch for those, so this is still blocked.

Copy link
Contributor

@ralren ralren left a comment

Choose a reason for hiding this comment

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

Looks good. All the different apps ran fine.

@jim
Copy link
Contributor Author

jim commented Apr 23, 2019

@pjdufour-truss Can you take a look at this again when you have a minute? I think we're in good shape.

We might need to tag our fork pdfcpu to a version as there isn't a nice way to pin to a non-master branch and if possible I'd like to avoid special casing for a single dependency. Added code to handle this case.

@jim jim requested a review from pjdufour-truss April 25, 2019 14:52
@jim jim changed the title Update go dependencies not affected by the issue with googlesource repos Update go dependencies Apr 25, 2019
Copy link
Contributor

@pjdufour-truss pjdufour-truss left a comment

Choose a reason for hiding this comment

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

Awesome work. However, it looks like go.mod is still out of sync. Could you squash and rebase from master and then roll forward from a clean go.mod?

For example, cobra/viper/pflag should be tracking master instead are on tagged versions. Should the custom branches logic preceed the semver logic? Should we hard-code more versions in the program. Not to bloat this more, but we could have a simple requirements file that includes branch overrides.

@pjdufour-truss
Copy link
Contributor

Could you also look into using https://godoc.org/os/exec#CommandContext and maybe setting a default timeout for each package with a few retries? One of the go get commands stalled on me.

@jim jim force-pushed the jb-update-godeps branch from a942a7b to f724337 Compare April 25, 2019 18:39
@jim
Copy link
Contributor Author

jim commented Apr 25, 2019

@pjdufour-truss I didn't add any retries yet, but I did add a timeout as you suggested. If we find ourselves retrying a lot we can go ahead and put that in.

@jim jim requested a review from pjdufour-truss April 26, 2019 14:51
@jim jim closed this Apr 26, 2019
@jim jim reopened this Apr 26, 2019
@jim jim assigned jim and unassigned ralren Apr 26, 2019
@pjdufour-truss
Copy link
Contributor

Can we remove gosec (or comment it out with TODO) since it wasn't working with modules yet?

@pjdufour-truss
Copy link
Contributor

It looks like the newest version of pop changed UUID libraries. Can we confirm that's not an issue? gobuffalo/pop#348

@jim
Copy link
Contributor Author

jim commented Apr 26, 2019

@pjdufour-truss

It looks like the newest version of pop changed UUID libraries. Can we confirm that's not an issue? gobuffalo/pop#348

It looks like we're already on github.com/gofrs/uuid v3.2.0+incompatible on master. This is just a different fork of the same original codebase with only minimal API changes compared to the gobuffalo one. The app and tests run so I think we're OK here.

Can we remove gosec (or comment it out with TODO) since it wasn't working with modules yet?

Sure thing. I'll make that change. Thanks for mentioning it.

@jim jim dismissed pjdufour-truss’s stale review April 29, 2019 16:02

Requested changes have been made.

@jim jim merged commit f5e195c into master Apr 29, 2019
@jim jim deleted the jb-update-godeps branch April 29, 2019 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants