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

Bump some deps #732

Merged
merged 7 commits into from
Oct 26, 2021
Merged

Bump some deps #732

merged 7 commits into from
Oct 26, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 20, 2021

  1. validation/.gitignore: fix

    There are many .t files in subdirectories. Fix gitignore accordingly.

    Closes: Ignore the *.t files because of compiled files. #714

  2. deps: switch to google/uuid

    It looks like satori/go.uuid has changed its API in the past (returning
    two values rather than one from New), and it is not maintained since
    around 2018.

    Switch to google/uuid which seems to be well maintained.

  3. deps: bump sirupsen/logrus to v1.8.1

  4. deps bump github.com/mrunalp/fileutils to v0.5.0

  5. deps: bump github.com/opencontainers/selinux to v1.9.1

  6. deps: github.com/hashicorp/go-multierror to v1.1.1

  7. deps: bump github.com/syndtr/gocapability to latest

    Also, fix a deprecation warning from NewPid.

@kolyshkin kolyshkin force-pushed the deps branch 5 times, most recently from 707a37c to 1b2280f Compare October 20, 2021 20:21
@kolyshkin
Copy link
Contributor Author

OK, non-trivial part of this separated to #733

@kolyshkin kolyshkin changed the title Bump some deps, fix some bugs Bump some deps Oct 20, 2021
@rhatdan
Copy link
Contributor

rhatdan commented Oct 25, 2021

LGTM
@mrunalp @vbatts @crosbymichael @dqminh PTAL

tap "github.com/mndrix/tap-go"
"github.com/mrunalp/fileutils"
rspec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/runtime-tools/generate"
"github.com/opencontainers/runtime-tools/specerror"
"github.com/opencontainers/runtime-tools/validation/util"
uuid "github.com/satori/go.uuid"
Copy link
Member

Choose a reason for hiding this comment

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

but why tho?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The appropriate commit message explains it (alas github downplays individual commits in its UI).

Here's a copy-paste to save you some clicks:

deps: switch to google/uuid

It looks like satori/go.uuid has changed its API in the past (returning
two values rather than one from New), and it is not maintained since
around 2018.

Switch to google/uuid which seems to be well maintained.

To reiterate, the New method used to return uuid, and if you try to bump
the module (github.com/satori/go.uuid) you'll see that it now returns uuid and error,
this means we have to add error check to all the callers. Switching to google/uuid
is simpler than making this change, with the added benefit of google one being well
maintained.

I think it was originally suggested by @thaJeztah in some issue or PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this clarification.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I recall Justin made a round of some PRs to replace some UUID libraries with this one (for another UUID library which had a vulnerability); https://github.com/pulls?q=is%3Apr+author%3Ajustincormack+uuid+is%3Aclosed

And notary switched to the UUID library from distribution/distribution (don't think we want to add that as dependency though); moby/moby#38868

So, given that google/uuid looks to be used in various codebases in the ecosystem, it's probably an OK choice

@tianon
Copy link
Member

tianon commented Oct 25, 2021

(Explanation sounds good to me, but I'll let @vbatts confirm whether he's satisfied 😄)

@kolyshkin

This comment has been minimized.

There are many .t files in subdirectories. Fix gitignore accordingly.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It looks like satori/go.uuid has changed its API in the past (returning
two values rather than one from New), and it is not maintained since
around 2018.

Switch to google/uuid which seems to be well maintained.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, fix a deprecation warning from NewPid.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

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

LGTM
the only failed check is not related to changes here, but I've opened an issue to track it #737

@vbatts vbatts merged commit 181da7f into opencontainers:master Oct 26, 2021
github.com/sirupsen/logrus v1.8.1
github.com/stretchr/testify v1.3.0
github.com/syndtr/gocapability v0.0.0-20170704070218-db04d3cc01c8
github.com/urfave/cli v1.19.1
github.com/xeipuuv/gojsonschema v1.2.0
golang.org/x/sys v0.0.0-20191026070338-33540a1f6037
golang.org/x/sys v0.0.0-20191115151921-52ab43148777
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should update this one to a current version as well for Go 1.17 compatibility (see docker/cli#3269)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an implicit upgrade (caused by github.com/opencontainers/selinux bump). We can surely do an explicit update, too.

@kolyshkin kolyshkin mentioned this pull request Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants