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.

Sign up for GitHub

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

Jump to bottom

Stabilize ec2_eip module #936

Merged
merged 3 commits into from
Feb 16, 2022
Merged

Stabilize ec2_eip module #936

merged 3 commits into from
Feb 16, 2022

Conversation

jatorcasso
Copy link
Contributor

@jatorcasso jatorcasso commented Feb 11, 2022 •
edited
Loading

SUMMARY
  • fixed check_mode issues
  • added integration tests for check_mode / idempotency
  • updated json returned when state = absent for clarity
  • removed json_query references
  • fixes Unstable integration tests ec2_eip #159

Depends-On: ansible-collections/amazon.aws#672

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_eip

@ansibullbot
Copy link

cc @iiibrad @jillr @markuman @RickMendes @s-hertel @tremble
click here for bot help

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Feb 11, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

alinabuzachis
alinabuzachis reviewed Feb 14, 2022
View reviewed changes
plugins/modules/ec2_eip.py Outdated Show resolved Hide resolved
plugins/modules/ec2_eip.py
result = {
'changed': changed
}
module.exit_json(**result)

result['changed'] |= ensure_ec2_tags(
Copy link
Contributor

@alinabuzachis alinabuzachis Feb 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since allocate_address supports TagsSpecifications (https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ec2.html#EC2.Client.associate_address), you can directly add the tags on creation instead of after associating the ip. Just have a look here for example https://github.com/ansible-collections/amazon.aws/pull/531/files

TagSpecifications

Copy link
Contributor

Choose a reason for hiding this comment

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

not available until 1.19.41
While we should be able to support this in 4.0.0 (1.20.0 was released Feb 2021), it's not something we officially support yet. I'd recommend pulling this into a separate PR

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@alinabuzachis
Copy link
Contributor

Can you also please have a look at this #159 and link it if your changes solve the bugs?

@jatorcasso
Copy link
Contributor Author

Can you also please have a look at this #159 and link it if your changes solve the bugs?

I commented out unstable and I wasn’t having issues… I wanted to see how it performed in CI first. I didn’t make any changes that would’ve fixed that afaik.

@jillr
Copy link
Collaborator

jillr commented Feb 15, 2022

@jatorcasso if you want to see if the stability of the tests has improved I run the tests on the CI account from my local system in a screen or tmux session (with history size set to something very large, like 10,000,000 lines so you can review failures no matter how far back they are) with a shell script like this, to see if any failures are produced over a large number of executions:

$ aws_tester.sh ec2_eip
$ cat ~/src/aws_tester-sh
#!/bin/bash

MAX_TRIES=50
COUNT=0

while [ $COUNT -lt $MAX_TRIES ]; do
#    /usr/bin/time -a -o /tmp/timer.txt -f %E ansible-test integration ${1} --docker --allow-disabled  --allow-unsupported 
    # Use a random word we can be certain will be unique in our screen history that we can search scrollback for
    # Test output can be quite busy otherwise  :)
    echo -e "Broccoli Count is ${COUNT}"
    ansible-test integration ${1} --docker  --allow-disabled --allow-unsupported --allow-unstable -vvv --python 3.7
    if [ ${?} -ne 0 ]; then echo ${COUNT} >> /tmp/s3_bucket_fails ; fi
    COUNT=$(($COUNT+1))
done

@jillr jillr added the mergeit Merge the PR (SoftwareFactory) label Feb 15, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit d0596e3 into ansible-collections:main Feb 16, 2022
@markuman markuman mentioned this pull request Feb 22, 2022
Merged
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Feb 23, 2022
@markuman
Stabilize ec2_eip module (#936) (#946)
a54d252 
backport-3: Stabilize ec2_eip module (#936) 

Backport into stable-3 of #936 with commit id: d0596e3
SUMMARY
fixed check_mode issues
added integration tests for check_mode / idempotency
updated json returned when state = absent for clarity
removed json_query references
fixes #159
Depends-On: ansible-collections/amazon.aws#672
ISSUE TYPE
Feature Pull Request
COMPONENT NAME
ec2_eip
Reviewed-by: Mark Woolley 
Reviewed-by: Mark Chappell 
Reviewed-by: Alina Buzachis 
Reviewed-by: Joseph Torcasso 
Reviewed-by: Jill R 
SUMMARY


ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request
New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION
@renovate renovate bot mentioned this pull request Apr 6, 2022
Merged
1 task
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
@jatorcasso @goneri
Stabilize ec2_eip module (ansible-collections#936)
bad0d88 
Stabilize ec2_eip module

SUMMARY

fixed check_mode issues
added integration tests for check_mode / idempotency
updated json returned when state = absent for clarity
removed json_query references
fixes ansible-collections#159

Depends-On: ansible-collections/amazon.aws#672
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
ec2_eip

Reviewed-by: Mark Woolley <mw@marknet15.com>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Alina Buzachis <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Jill R <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@d0596e3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremble tremble tremble left review comments

@jillr jillr jillr approved these changes

@marknet15 marknet15 marknet15 approved these changes

@alinabuzachis alinabuzachis Awaiting requested review from alinabuzachis

Assignees
No one assigned
Labels
community_review feature This issue/PR relates to a feature request integration tests/integration mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unstable integration tests ec2_eip
6 participants
@jatorcasso @ansibullbot @alinabuzachis @jillr @tremble @marknet15