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

fix: use moby/term rather than our own term package #227

Merged
merged 1 commit into from
Oct 11, 2020

Conversation

bakins
Copy link
Contributor

@bakins bakins commented Oct 2, 2020

Related issue

#212
#208

Proposed changes

Use https://github.com/moby/term rather than the term package introduced in #122

There is only a single call to term in the code base at

ws, err := term.GetWinsize(p.terminalFd)

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)

Further comments

I considered editing docker/pkg/term to include the fixes from moby/term#16 but dockertest only has a single call to the term package.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2020

CLA assistant check
All committers have signed the CLA.

@bakins bakins changed the title Use moby/term rather than our own term package fix: use moby/term rather than our own term package Oct 2, 2020
@aeneasr
Copy link
Member

aeneasr commented Oct 2, 2020

So originally I added docker as a direct dependency because there was no other way of consuming the Docker API without writing a lot of curl myself. The problem was however, that Docker pulled quite a lot of code that wasn't actually used. I could not find a way (at the time) to prevent those unused modules from being pulled. I still think it's not possible.

However, maybe today there is a much better option to talk to Docker's API? In which case we would probably also circumvent all the build issues and stuff! Maybe you know this?

@bakins
Copy link
Contributor Author

bakins commented Oct 2, 2020

@aeneasr I haven't looked at the full dockerclient in a while. I was mostly interested in fixing the issue with having to pin sys across a large number of projects where I use dockertest.

I don't mind looking into docker client, but I just wanted to offer up a PR to fix the current pressing issue quickly :) This PR only pulls in moby/term, which does not pull in any additional dependencies beyond what dockertest already depends on: https://github.com/ory/dockertest/pull/227/files#diff-37aff102a57d3d7b797f152915a6dc16

@bakins
Copy link
Contributor Author

bakins commented Oct 7, 2020

It seems the "official" dockerclient still pulls in a large number of dependencies. The moby/term package in this PR does not pull in any additional dependencies.

I'd like to not have to pin github.com/x/sys/ to a specific version anymore. If the approach in this PR is not acceptable, what alternative would be? I'm happy to do a PR.

Thanks!

@aeneasr
Copy link
Member

aeneasr commented Oct 11, 2020

Ok, sounds good and sorry for the late review, it was a busy week :)

@aeneasr aeneasr merged commit 012ed9a into ory:v3 Oct 11, 2020
@bakins bakins deleted the use-moby-term branch October 12, 2020 14:21
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.

3 participants