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

Consolidate the dependencies for the IsTerminal() API #16292

Merged
merged 1 commit into from
Oct 27, 2022
Merged

Consolidate the dependencies for the IsTerminal() API #16292

merged 1 commit into from
Oct 27, 2022

Conversation

debarshiray
Copy link
Member

@debarshiray debarshiray commented Oct 25, 2022

The rest of the code has been using golang.org/x/* for the IsTerminal() API for a long time, not github.com/mattn/go-isatty. It seems better to stick to packages from the golang.org domain, whenever possible, and one less dependency is always a good thing.

The modules were not cleaned up with make vendor because go mod tidy was running into:

go: error loading go 1.16 module graph:
  github.com/containers/image/v5@v5.23.1-0.20221015133641-1921a1993c67
  requires
      github.com/honeycombio/beeline-go@v1.9.0 requires
      github.com/mattn/go-sqlite3@v2.0.3+incompatible: reading
        github.com/mattn/go-sqlite3/go.mod at revision v2.0.3: unknown
        revision v2.0.3

If reproducibility with go 1.16 is not needed:
    go mod tidy -compat=1.17

Since go.mod already requires Go 1.17, the following commands were manually run:

$ go mod tidy -compat=1.17
$ go mod vendor
$ go mod verify

Further modifications to go.sum were done manually based on the complaints from postbuild.sh run by the CI.

Fixes: 85db895 ("logging: new mode -l passthrough")

Does this PR introduce a user-facing change?

None

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2022

Nice
-307 lines of code.
LGTM
/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debarshiray, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2022
@mheon
Copy link
Member

mheon commented Oct 25, 2022

Code LGTM.Tests red. Restarted in case it's a flake.

@edsantiago
Copy link
Collaborator

buildah bud test failures are not flakes, but they have nothing whatsoever to do with this PR: something seems to have changed in the ubi8 image.

I'm just going to drop this here and step away for the day: containers/buildah#4318. Good luck.

The rest of the code has been using golang.org/x/* for the IsTerminal()
API for a long time, not github.com/mattn/go-isatty.  It seems better to
stick to packages from the golang.org domain, whenever possible, and one
less dependency is always a good thing.

The modules were not cleaned up with 'make vendor' because 'go mod tidy'
was running into:

go: error loading go 1.16 module graph:
  github.com/containers/image/v5@v5.23.1-0.20221015133641-1921a1993c67
  requires
      github.com/honeycombio/beeline-go@v1.9.0 requires
      github.com/mattn/go-sqlite3@v2.0.3+incompatible: reading
        github.com/mattn/go-sqlite3/go.mod at revision v2.0.3: unknown
        revision v2.0.3

If reproducibility with go 1.16 is not needed:
    go mod tidy -compat=1.17

Since go.mod already requires Go 1.17, the following commands were
manually run:
$ go mod tidy -compat=1.17
$ go mod vendor
$ go mod verify

Further modifications to go.sum were done manually based on the
complaints from postbuild.sh run by the CI.

[NO NEW TESTS NEEDED] as it's not a functional change.

Fixes: 85db895 ("logging: new mode -l passthrough")

Signed-off-by: Debarshi Ray <rishi@fedoraproject.org>
@mheon
Copy link
Member

mheon commented Oct 27, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2022
@openshift-merge-robot openshift-merge-robot merged commit 84d04a2 into containers:main Oct 27, 2022
@debarshiray debarshiray deleted the wip/rishi/dont-use-mattn-go-isatty branch November 2, 2022 11:42
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants