-
Notifications
You must be signed in to change notification settings - Fork 341
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
Verify and fully implement ec2_instance network option logic #2054
Labels
Comments
Fully agree on the |
softwarefactory-project-zuul bot
pushed a commit
that referenced
this issue
Jul 1, 2024
SUMMARY Closes #2054 JIRA: https://issues.redhat.com/browse/ACA-1382 ISSUE TYPE Bugfix Pull Request Feature Pull Request COMPONENT NAME ec2_instance Reviewed-by: GomathiselviS Reviewed-by: Alina Buzachis Reviewed-by: Bikouo Aubin Reviewed-by: Mark Chappell
patchback bot
pushed a commit
that referenced
this issue
Aug 28, 2024
SUMMARY Closes #2054 JIRA: https://issues.redhat.com/browse/ACA-1382 ISSUE TYPE Bugfix Pull Request Feature Pull Request COMPONENT NAME ec2_instance Reviewed-by: GomathiselviS Reviewed-by: Alina Buzachis Reviewed-by: Bikouo Aubin Reviewed-by: Mark Chappell (cherry picked from commit f7d7ffa)
tremble
pushed a commit
that referenced
this issue
Aug 28, 2024
SUMMARY Closes #2054 JIRA: https://issues.redhat.com/browse/ACA-1382 ISSUE TYPE Bugfix Pull Request Feature Pull Request COMPONENT NAME ec2_instance Reviewed-by: GomathiselviS Reviewed-by: Alina Buzachis Reviewed-by: Bikouo Aubin Reviewed-by: Mark Chappell (cherry picked from commit f7d7ffa) Co-authored-by: Bikouo Aubin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
While reviewing this bugfix PR, it seems like the
build_network_spec
function still needs more work to fully implement the logic in the API's network interface specification (as evidenced by the function'sTODO: more special snowflake network things
comment). For example, many of the suboptions have limitations on the AWS side such as "You cannot specify this option if you're launching more than one instance in a RunInstances request" and "Applies only if creating a network interface when launching an instance". We should verify which limitations apply, ensure they are enforced in our code, and where possible add tests for this.In addition, I find the entire
network
option for this module to be a bit confusing as it requires either "a dictionary containing the key C(interfaces) corresponding to a list of network interface IDs or containing specifications for a single network interface." While this would potentially result in breaking changes to the module, I think it's worth considering whether there are approaches that would be more clear for both the end user and the internal logic. For example we could update the currentnetwork
option to just take a list of interfaces (with the ENI ID as one possible suboption for each list element), and/or we could add a new option to allow providing a list of ENI IDs separate from the option for a single network interface's specifications. Other ideas welcome, these are just my initial thoughts.Issue Type
Feature Idea
Component Name
ec2_instance
Additional Information
Code of Conduct
The text was updated successfully, but these errors were encountered: