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

Bug 1912565: update moby/term dependency #1918

Merged
merged 1 commit into from
Jan 5, 2021
Merged

Bug 1912565: update moby/term dependency #1918

merged 1 commit into from
Jan 5, 2021

Conversation

exdx
Copy link
Member

@exdx exdx commented Dec 22, 2020 •
edited
Loading

Description of the change:
Updates the indirect moby/term dependency to a newer version due to the following error on macOS only when running commands like make run-local and make unit

vendor/github.com/docker/docker/pkg/term/tc.go:13:28: undefined: unix.SYS_IOCTL
vendor/github.com/docker/docker/pkg/term/tc.go:18:28: undefined: unix.SYS_IOCTL
vendor/github.com/docker/docker/pkg/term/termios_bsd.go:24:31: undefined: unix.SYS_IOCTL
vendor/github.com/docker/docker/pkg/term/termios_bsd.go:37:31: undefined: unix.SYS_IOCTL
# github.com/moby/term
vendor/github.com/moby/term/tc.go:13:28: undefined: unix.SYS_IOCTL
vendor/github.com/moby/term/tc.go:18:28: undefined: unix.SYS_IOCTL
vendor/github.com/moby/term/termios_bsd.go:24:31: undefined: unix.SYS_IOCTL
vendor/github.com/moby/term/termios_bsd.go:37:31: undefined: unix.SYS_IOCTL

The docker term library attempts to make syscalls directly, which is not supported in recent versions of macOS.
See ory/dockertest#212 for more info

Note: you can update a transitive dependency without using replaces so long as the version is compatible via the go tool.

As part of maintaining the require statements in go.mod, the go command tracks which ones provide packages imported directly by the current module and which ones provide packages only used indirectly by other module dependencies. Requirements needed only for indirect uses are marked with a "// indirect" comment in the go.mod file. Indirect requirements are automatically removed from the go.mod file once they are implied by other direct requirements. Indirect requirements only arise when using modules that fail to state some of their own dependencies or when explicitly upgrading a module's dependencies ahead of its own stated requirements.

Motivation for the change:
Fixes broken local testing on macOS

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@exdx exdx requested review from njhale and dinhxuanvu December 22, 2020 18:39
dinhxuanvu
dinhxuanvu approved these changes Dec 22, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020
@exdx
Copy link
Member Author

exdx commented Dec 22, 2020

rebased off 1.20 bump

dinhxuanvu
dinhxuanvu approved these changes Dec 22, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@njhale
Copy link
Member

njhale commented Dec 23, 2020

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2020
dinhxuanvu
dinhxuanvu approved these changes Dec 23, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, exdx, njhale

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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@benluddy
Copy link
Contributor

/hold

Can we pin github.com/moby/term instead?

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@exdx exdx changed the title fix: pin golang.org/x/sys package fix: update moby/term dependency Dec 23, 2020
@benluddy
Copy link
Contributor

/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 23, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@exdx exdx changed the title fix: update moby/term dependency Bug 1912565: update moby/term dependency Jan 4, 2021
@exdx
Copy link
Member Author

exdx commented Jan 4, 2021

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jan 4, 2021
@openshift-ci-robot
Copy link
Collaborator

@exdx: This pull request references Bugzilla bug 1912565, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1912565: update moby/term dependency

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

@exdx: This pull request references Bugzilla bug 1912565, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit ad06749 into operator-framework:master Jan 5, 2021
@openshift-ci-robot
Copy link
Collaborator

@exdx: All pull requests linked via external trackers have merged:

Bugzilla bug 1912565 has been moved to the MODIFIED state.

In response to this:

Bug 1912565: update moby/term dependency

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@njhale njhale Awaiting requested review from njhale

@awgreene awgreene Awaiting requested review from awgreene

Assignees

@benluddy benluddy

@dinhxuanvu dinhxuanvu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@exdx @njhale @openshift-ci-robot @openshift-bot @benluddy @dinhxuanvu @openshift-merge-robot