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 EC2 classic bootstrap #642

Merged
merged 3 commits into from
Mar 28, 2020
Merged

Fix EC2 classic bootstrap #642

merged 3 commits into from
Mar 28, 2020

Conversation

josacar
Copy link
Contributor

@josacar josacar commented Mar 26, 2020

Description

Fixes the described scenario in #641

Related Issue

Fixes #641 641

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@josacar
Copy link
Contributor Author

josacar commented Mar 26, 2020

I haven't added tests for this bugfix, but I fixed the failing ones.

Let me know if you need me to add specific tests.

@tas50
Copy link
Contributor

tas50 commented Mar 26, 2020

@josacar This is very cool. Can you sign the commit off for the DCO and see if there's anything you can do to test it.

@joseluis-fw joseluis-fw force-pushed the fix-ec2-classic branch 3 times, most recently from 488caaf to 08bd696 Compare March 26, 2020 20:18
@josacar
Copy link
Contributor Author

josacar commented Mar 26, 2020

@tas50 I've added a test for EIP attaching, but I cannot find any existing test for server_hashes function. Let me know what you think.

Signed-off-by: Jose Luis Salas <[email protected]>
@joseluis-fw joseluis-fw force-pushed the fix-ec2-classic branch 3 times, most recently from 81c597f to 5721708 Compare March 27, 2020 07:34
@@ -29,6 +29,7 @@
let(:ec2_connection) { Aws::EC2::Client.new(stub_responses: true) }
let(:ebs) { OpenStruct.new(volume_size: 30, iops: 123) }
let(:groups) { [OpenStruct.new(name: "grp-646rswg")] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the docs, this mock should be wrong, and it should be group_name instead of name:

resp.reservations[0].groups #=> Array
resp.reservations[0].groups[0].group_name #=> String
resp.reservations[0].groups[0].group_id #=> String

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this only makes sense to EC2 classic as VPC uses security_group_ids instead of groups

@josacar
Copy link
Contributor Author

josacar commented Mar 28, 2020

I've introduced instance_double as they raise an exception when a method does not exist in the mocked class, as opposite as Openstruct that returns nil. This makes less fragile tests as mocked classes evolve and mocks get obsolete.

@tas50
Copy link
Contributor

tas50 commented Mar 28, 2020

Thanks for the tests @josacar

@tas50 tas50 merged commit 98a459f into chef:master Mar 28, 2020
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.

EC2 Classic provision is broken
2 participants