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 not using the ConnectionHelper Host for SSH connections #637

Merged
merged 1 commit into from
May 16, 2023

Conversation

limond
Copy link
Contributor

@limond limond commented May 15, 2023

This PR changes the patches/0003-smarter-host-resolution.patch to respect the host of the ssh ConnectionHelper as it is used in the upstream provider and in this repository before 0b2969f.

This fixes the bug #635 that leads to errors when using an SSH host with a username like:

const provider = new docker.Provider("provider", {
  host: "ssh://limond@localhost:22",
});

const exampleNginx = new docker.Container("example-nginx", {
  image: "nginx:latest",
  ports: [{
    internal: 80,
    external: 80,
  }],
}, { provider });

I was unsure if changing the patch file or generating a new patch would be the right approach.

@github-actions
Copy link

PR is now waiting for a maintainer to run the acceptance tests.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@t0yv0
Copy link
Member

t0yv0 commented May 16, 2023

Thanks a ton for submitting a PR here! Will be taking it for a spin this week.

@t0yv0 t0yv0 requested review from guineveresaenger and removed request for t0yv0 May 16, 2023 17:24
@guineveresaenger
Copy link
Contributor

guineveresaenger commented May 16, 2023

Hi @limond - thank you so much for this fix! Yes, it looks like we skipped this functionality in our patch, and I think it is fine to alter the patch here.

@guineveresaenger
Copy link
Contributor

/run-acceptance-tests

@pulumi-bot
Copy link
Contributor

Copy link
Contributor

@guineveresaenger guineveresaenger left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much!

@guineveresaenger guineveresaenger merged commit 4c64c91 into pulumi:master May 16, 2023
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.

4 participants