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

Refreshing devices list after adding a disk or cdrom controller #7167

Merged
merged 1 commit into from
Jun 29, 2016

Conversation

dkalleg
Copy link
Contributor

@dkalleg dkalleg commented Jun 14, 2016

Without refreshing the devices list, we can't find the controller we just created.

@dkalleg
Copy link
Contributor Author

dkalleg commented Jun 17, 2016

@chrislovecnm @stack72 @phinze
Adding folks for visibility, please review at your convenience.

controller, err = devices.FindDiskController(controller_type)
if err != nil {
return fmt.Errorf("[ERROR] Could not find the controller we just created")
return fmt.Errorf("[ERROR] Could not find the new %v controller: %v", controller_type, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

please log err, or check that it is logged in stack above.

@chrislovecnm
Copy link
Contributor

@dkalleg is this an edge case or should have we caught this in testing?

@dkalleg dkalleg force-pushed the controllerCreateFix branch 2 times, most recently from d72565d to 11dc327 Compare June 22, 2016 00:06
Also taking out an unnecessary ForceNew on controller_type as well
as correcting a reference to controller_type in Update case.
@thetuxkeeper
Copy link
Contributor

@dkalleg : Interesting. It's the same way I do it in #7154 for the network interfaces. Little sad that we have to spam api calls (create device function with created device as return value would be great)
@chrislovecnm : Since it's all before the Read() I think it's "just" a bug. the tfstate will include the created controller, so the tests won't see it.
Additionally I think (not tested) it will only fail if we add a controller type that wasn't present before (will find the old/wrong one). So we need a test that adds a non-default controller (I think a VM has a controller automatically, should be scsi) or make sure that we have no controllers at all and add one.

@dkalleg
Copy link
Contributor Author

dkalleg commented Jun 26, 2016

@stack72 @phinze @thetuxkeeper @chrislovecnm @jen20
Any actionable feedback?

@chrislovecnm
Copy link
Contributor

LGTM!!

@stack72 stack72 merged commit ab92fa4 into hashicorp:master Jun 29, 2016
@dkalleg
Copy link
Contributor Author

dkalleg commented Jun 30, 2016

Thanks @stack72

@dagnello dagnello deleted the controllerCreateFix branch September 21, 2016 18:33
@ghost
Copy link

ghost commented Apr 22, 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 Apr 22, 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.

5 participants