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

Rewrite of cloud.py with a new licence #488

Merged
merged 4 commits into from
Sep 15, 2021
Merged

Rewrite of cloud.py with a new licence #488

merged 4 commits into from
Sep 15, 2021

Conversation

abikouo
Copy link
Contributor

@abikouo abikouo commented Sep 3, 2021 •
edited by jillr
Loading

SUMMARY

Implement the specified functionality without referencing the plugins/module_utils/cloud.py file or tests, under BSD license.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

Depends-On: #503

@abikouo
Copy link
Contributor Author

abikouo commented Sep 3, 2021

@jillr could you please have a look and see if there are missing features?

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request module_utils module_utils needs_triage new_plugin New plugin plugins plugin (any type) tests tests labels Sep 3, 2021
@tremble
Copy link
Contributor

tremble commented Sep 3, 2021

check ondemand

@abikouo abikouo force-pushed the rewrite_cloud_py branch 2 times, most recently from 9ba549e to 3ea795d Compare September 3, 2021 15:52
jillr
jillr reviewed Sep 3, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Thanks for this work @abikouo. I've haven't dug into your CI failures but you might start by looking at your staticmethods (I could be wrong though, just a guess).

plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
@jillr
Copy link
Collaborator

jillr commented Sep 3, 2021

@goneri I believe you also have familiarity with this and have examined the original cloud.py, could you please add a review?

@abikouo
Copy link
Contributor Author

abikouo commented Sep 6, 2021

recheck

4 similar comments
@abikouo
Copy link
Contributor Author

abikouo commented Sep 6, 2021

recheck

@abikouo
Copy link
Contributor Author

abikouo commented Sep 7, 2021

recheck

@abikouo
Copy link
Contributor Author

abikouo commented Sep 7, 2021

recheck

@abikouo
Copy link
Contributor Author

abikouo commented Sep 7, 2021

recheck

@jillr jillr mentioned this pull request Sep 7, 2021
Closed
jillr
jillr approved these changes Sep 7, 2021
View reviewed changes
Copy link
Collaborator

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Other than some unrelated CI failures this is looking good on both this repo and community.aws. I'd like to get a few more reviews before we merge though, thanks @abikouo!

@jillr jillr requested review from goneri and tremble September 7, 2021 23:58
@alinabuzachis
Copy link
Collaborator

recheck

tremble
tremble requested changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for this work @abikouo

Generally looks good. I've a few concerns inline, @jillr feel free to overrule me if you think I'm being excessively picky here.

I'd like to see a test for max retries, it would be all to easy to rewrite and fail after x-1 or x+1 attempts rather than x attempts.

plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
@abikouo abikouo changed the title DNM - Re-implementation of cloud.py based on specs Rewrite of cloud.py with a new licence Sep 8, 2021
@abikouo abikouo requested a review from tremble September 8, 2021 16:58
tremble
tremble approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the changes. I'd still like to see a test that retries is properly respected, but otherwise LGTM.

goneri
goneri requested changes Sep 8, 2021
View reviewed changes
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
plugins/module_utils/cloud_py.py Outdated Show resolved Hide resolved
tests/unit/module_utils/test_cloud.py Show resolved Hide resolved
tests/unit/module_utils/test_cloud.py Show resolved Hide resolved
@abikouo
Copy link
Contributor Author

abikouo commented Sep 10, 2021

recheck

@goneri goneri added the gate label Sep 10, 2021
ansible-zuul[bot]
ansible-zuul bot approved these changes Sep 10, 2021
View reviewed changes
Copy link
Contributor

@ansible-zuul ansible-zuul bot left a comment

Choose a reason for hiding this comment

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

LGTM!

@abikouo
Copy link
Contributor Author

abikouo commented Sep 10, 2021

recheck

@tremble tremble removed the gate label Sep 10, 2021
@tremble
Copy link
Contributor

tremble commented Sep 10, 2021

Before gating we need to move this back from cloud.py.py to cloud.py

@tremble tremble mentioned this pull request Sep 11, 2021
Merged
@ansibullbot ansibullbot removed the new_plugin New plugin label Sep 11, 2021
@tremble
Copy link
Contributor

tremble commented Sep 11, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Sep 13, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Sep 13, 2021

The sanity failures will need to be fixed (metaclass and future import). The ec2_eni failure is a waiter timeout thhat rechecks should help with and the ec2_metadata_facts timeout should be addressed by #498 when it merges (oh Amazon, you and your latency and eventual consistency :)).

@jillr
Copy link
Collaborator

jillr commented Sep 14, 2021 •
edited
Loading

recheck

3 similar comments
@jillr
Copy link
Collaborator

jillr commented Sep 14, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Sep 14, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Sep 14, 2021

recheck

@jillr
Copy link
Collaborator

jillr commented Sep 15, 2021

Every CI failure since the sanity fix has been a timeout (either zuul or aws). We're running the collection's full set of integration tests which is always (in the current config) at risk of hitting zuul's 1 hour time out. We've also been seeing substantial AWS latency the last couple says (ie; 15 minutes to boot instances in us-east-1). The effort needed to get a 2 test runs (check + gate) to pass exceeds the value of continuing to recheck. We have confidence that we've seen enough test successes over the course of our retries that all tests are actually green (if ansible-test-cloud-integration-aws-py36_2 fails on a timeout one time, and ansible-test-cloud-integration-aws-py36_4 is the thing that fails the next time, technically we've seen green results from both sets of tests).

I've temporarily disabled branch protection and am bypassing zuul to merge this PR.

@jillr jillr merged commit f3193c7 into ansible-collections:main Sep 15, 2021
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@Surgo
Support new enableExecuteCommand options for ECS service (ansible-col…
97f757a 
…lections#488)

Support new enableExecuteCommand options for ECS service

SUMMARY

Support new ecs exec feature for ECS service

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
ecs_service
ADDITIONAL INFORMATION



Create ECS service with enable_execute_command option,
- name: create exec service
  ecs_service:
    state: present
    ...
    enable_execute_command: true

and we can exec ECS task
$ aws ecs execute-command --cluster xxxxx --task arn:aws:ecs:us-east-1:*****:task/webapp/***** --container xxxxx --interactive --command /bin/bash


The Session Manager plugin was installed successfully. Use the AWS CLI to start a session.


Starting session with SessionId: ecs-execute-command-0c17f94b36227381f
root@ip-10-0-66-68:/#

Reviewed-by: Mark Chappell
Reviewed-by: Alina Buzachis
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
@Surgo @abikouo
Support new enableExecuteCommand options for ECS service (ansible-col…
8a3c37d 
…lections#488)

Support new enableExecuteCommand options for ECS service

SUMMARY

Support new ecs exec feature for ECS service

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
ecs_service
ADDITIONAL INFORMATION



Create ECS service with enable_execute_command option,
- name: create exec service
  ecs_service:
    state: present
    ...
    enable_execute_command: true

and we can exec ECS task
$ aws ecs execute-command --cluster xxxxx --task arn:aws:ecs:us-east-1:*****:task/webapp/***** --container xxxxx --interactive --command /bin/bash


The Session Manager plugin was installed successfully. Use the AWS CLI to start a session.


Starting session with SessionId: ecs-execute-command-0c17f94b36227381f
root@ip-10-0-66-68:/#

Reviewed-by: Mark Chappell
Reviewed-by: Alina Buzachis
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
@Surgo
Support new enableExecuteCommand options for ECS service (ansible-col…
3ad3b97 
…lections#488)

Support new enableExecuteCommand options for ECS service

SUMMARY

Support new ecs exec feature for ECS service

ISSUE TYPE

Feature Pull Request

COMPONENT NAME
ecs_service
ADDITIONAL INFORMATION



Create ECS service with enable_execute_command option,
- name: create exec service
  ecs_service:
    state: present
    ...
    enable_execute_command: true

and we can exec ECS task
$ aws ecs execute-command --cluster xxxxx --task arn:aws:ecs:us-east-1:*****:task/webapp/***** --container xxxxx --interactive --command /bin/bash


The Session Manager plugin was installed successfully. Use the AWS CLI to start a session.


Starting session with SessionId: ecs-execute-command-0c17f94b36227381f
root@ip-10-0-66-68:/#

Reviewed-by: Mark Chappell
Reviewed-by: Alina Buzachis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goneri goneri goneri approved these changes

@ansible-zuul ansible-zuul[bot] ansible-zuul[bot] approved these changes

@jillr jillr jillr approved these changes

@tremble tremble tremble approved these changes

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

Successfully merging this pull request may close these issues.

module_utils/cloud.py is licensed GPLv3+
6 participants
@abikouo @tremble @jillr @alinabuzachis @goneri @ansibullbot