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

Vpnaas: VPN service resource #300

Merged

Conversation

simonre
Copy link
Contributor

@simonre simonre commented Apr 27, 2018

Part 2 of a series that addresses #2

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 27, 2018

Build failed.

@simonre
Copy link
Contributor Author

simonre commented Apr 27, 2018

@jtopjian Unless I'm misinterpreting the openlab logs I don't think that this is a problem with my code (?)

This PR has the same problem as gophercloud/gophercloud#784 with the update function of the service resource. I put in a test that succeeds on error instead of a 'real' test of the functionality although I'm not sure that this really makes sense.

Also apart from the same issue as #270 this is ready for review.

@jtopjian
Copy link
Contributor

Unless I'm misinterpreting the openlab logs I don't think that this is a problem with my code (?)

That's correct -- you can ignore the results of OpenLab acceptance tests with these VPNaaS PRs.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@simonre Two small error checks. In addition, a sidebar entry in openstack.erb will also need to be created.

I put in a test that succeeds on error instead of a 'real' test of the functionality although I'm not sure that this really makes sense.

I apologize - I didn't see this part earlier. On Monday, I'll ask some of the Terraform devs about what to do in this scenario. Maybe there's a way to easily mock or stub the test or just do some basic exercises of the attributes.

Delay: 0,
MinTimeout: 2 * time.Second,
}
_, err = stateConf.WaitForState()
Copy link
Contributor

Choose a reason for hiding this comment

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

if err != nil {
  return err
}

Delay: 0,
MinTimeout: 2 * time.Second,
}
_, err = stateConf.WaitForState()
Copy link
Contributor

Choose a reason for hiding this comment

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

if err != nil {
  return err
}

@simonre
Copy link
Contributor Author

simonre commented Apr 30, 2018

@jtopjian Apart from the test issue this is ready for review again.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Apr 30, 2018

Build succeeded.

@jtopjian
Copy link
Contributor

jtopjian commented May 1, 2018

@simonre I have no idea why I misread your notes twice.

Since the problem is only with updating, then I'm OK with omitting update tests. If we can test creation, then that's good enough and will exercise a good amount of code in this resource.

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 4, 2018

Build failed.

@simonre
Copy link
Contributor Author

simonre commented May 4, 2018

@jtopjian I removed the update test and rebased the code.

Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@simonre Looks good to me. Thank you :)

@jtopjian jtopjian merged commit 50768a9 into terraform-provider-openstack:master May 4, 2018
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.

None yet

2 participants