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 attaching keypair error due to missing keypair name #6720

Merged
merged 2 commits into from
Sep 19, 2018

Conversation

chhaj5236
Copy link
Collaborator

KeyPair is created in stepConfigAlicloudKeyPair and stored in Comm field, but stepAttachKeyPair uses config.Comm.SSHKeyPairName which is found out to be an empty string. This causes stepAttachKeyPair#Run returns immediately without actually attaching the KeyPair.

This fix passes Comm field to stepAttachKeyPair to make use of the KeyPair generated.

Copy link
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

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

Hello, thanks for the PR 🙂 , the actual problem here is that config is stored as a copy in the state bag.
config.Comm.SSHKeyPairName is updated during build steps; but since it's stored as a copy in the state bag it won't be updated.

So a nicer fix here would to change the state.Get("config").(Config) calls to be state.Get("config").(*Config) and

state.Put("config", b.config)

to state.Put("config", &b.config) too.

Thanks 🙂

@@ -21,7 +23,7 @@ func (s *stepAttachKeyPair) Run(_ context.Context, state multistep.StateBag) mul
config := state.Get("config").(Config)
Copy link
Contributor

Choose a reason for hiding this comment

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

This config should be a pointer so that you don't have to add a field to stepAttachKeyPair .

config := state.Get("config").(*Config)

@chhaj5236
Copy link
Collaborator Author

@azr I have done the change. That was a great suggestion and it did fix the issue.

azr added a commit that referenced this pull request Sep 18, 2018
@zhuzhih2017 zhuzhih2017 merged commit 9e3e2e8 into hashicorp:master Sep 19, 2018
@azr
Copy link
Contributor

azr commented Sep 19, 2018

Awesome, thanks people 🙂

@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants