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

Azure: Fixed ability to pass SSH key to Linux VMs #53238

Conversation

@nicholasmhughes
Copy link
Contributor

commented May 24, 2019

What does this PR do?

This PR fixes errors I found while testing the ability to stand up instances in Azure using an SSH key instead of a password.

What issues does this PR fix or reference?

N/A

Previous Behavior

Exception thrown looking for a password that isn't present in my config.

New Behavior

Successful instance creation using the latest Azure SDK

Tests written?

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@Ch3LL Ch3LL requested a review from gtmanfred Jun 18, 2019
@Ch3LL Ch3LL self-requested a review Jun 18, 2019
salt/cloud/clouds/azurearm.py Outdated Show resolved Hide resolved
@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Everything else looks good.

@cmcmarrow

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Thanks nicholasmhughes. Looks good to me!

Copy link
Contributor

left a comment

can we get a regression test added here so we can ensure this doesn't occur in the future?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I don't have the time to create tests right now. We could throw it out there to the world to see if someone wants to take on writing tests, but I'm not sure if you want to hold up this bug fix.

@Ch3LL Ch3LL added the Needs Testcase label Jul 2, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

How long has this been an issue? is this a recent regression?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

No idea. It's been a year since I've used this cloud driver, and it was in the develop branch at that time.

nicholasmhughes and others added 2 commits Jul 2, 2019
@Ch3LL Ch3LL requested a review from saltstack/team-core as a code owner Aug 8, 2019
@pull-assigner pull-assigner bot requested a review from garethgreenaway Aug 8, 2019
@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

Added azurearm tests here: #54172

And cherry-picked your changes here into 2019.2.1 here #54173 since the tests do not pass without your changes. thanks!

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

cool. we're good to close this now, right @Ch3LL ?

@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

I was actually hoping to merge this into 2019.2 at the same time we merge my PR in 2019.2.1.

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

gotcha. backs away slowly

@Ch3LL
Ch3LL approved these changes Aug 13, 2019
dwoz added a commit that referenced this pull request Aug 13, 2019
Cherry Pick #50567 and #53238 into 2019.2.1
@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

Going through my open PRs... @Ch3LL , is this still scheduled for merge?

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2019

@Ch3LL this master branch move has me confused... Since this code made it into 2019.2.1, which (I believe) was used for master, then we don't need this PR anymore... correct?

@waynew

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

@nicholasmhughes If this code was in 2019.2.1 then it should be in master (and you can always double check, of course)

If it's already in master, then yeah you can feel free to just close this PR. Thanks!

@nicholasmhughes

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.