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

Allow ECS.Client.delete_service to force delete a service #228

Merged
merged 2 commits into from
Oct 22, 2021
Merged

Allow ECS.Client.delete_service to force delete a service #228

merged 2 commits into from
Oct 22, 2021

Conversation

asaf400
Copy link
Contributor

@asaf400 asaf400 commented Sep 15, 2020

SUMMARY

Change allows ansible to forcefully delete a service,
required when deleting a service with >0 scale, or no target group

ISSUE TYPE

#220

COMPONENT NAME

ecs_service

@asaf400
Copy link
Contributor Author

asaf400 commented Sep 15, 2020

I do not know how to setup shippable for local debugging before submitting the PR, And I did not find any information about this.
I am sorry if this PR is currently in a state of garbage, while I iron out the tests for the changes.. (Help is appreciated)

@ansibullbot
Copy link

cc @Java1Guy @jillr @kaczynskid @s-hertel @tremble @wimnat @zacblazic
click here for bot help

@ansibullbot ansibullbot added affects_2.10 integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Sep 22, 2020
@ansibullbot
Copy link

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

plugins/modules/ecs_service.py:0:0: doc-required-mismatch: Argument 'force_deletion' in argument_spec is required, but is not documented as being required
plugins/modules/ecs_service.py:0:0: no-default-for-required-parameter: Argument 'force_deletion' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
plugins/modules/ecs_service.py:0:0: parameter-type-not-in-doc: Argument 'force_deletion' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/ecs_service.py:0:0: undocumented-parameter: Argument 'force_deletion' is listed in the argument_spec, but not documented in the module documentation

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

plugins/modules/ecs_service.py:0:0: no-default-for-required-parameter: Argument 'force_deletion' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
plugins/modules/ecs_service.py:0:0: parameter-type-not-in-doc: Argument 'force_deletion' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/ecs_service.py:0:0: undocumented-parameter: Argument 'force_deletion' is listed in the argument_spec, but not documented in the module documentation

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

plugins/modules/ecs_service.py:0:0: doc-required-mismatch: Argument 'force_deletion' in argument_spec is required, but is not documented as being required
plugins/modules/ecs_service.py:0:0: no-default-for-required-parameter: Argument 'force_deletion' in argument_spec is marked as required but specifies a default. Arguments with a default should not be marked as required
plugins/modules/ecs_service.py:0:0: parameter-type-not-in-doc: Argument 'force_deletion' in argument_spec defines type as 'bool' but documentation doesn't define type
plugins/modules/ecs_service.py:0:0: undocumented-parameter: Argument 'force_deletion' is listed in the argument_spec, but not documented in the module documentation

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI stale_ci CI is older than 7 days, rerun before merging labels Nov 16, 2020
@gundalow
Copy link
Contributor

@asaf400 You can run the tests locally, have a look at https://www.katacoda.com/ansible-community/scenarios/fixing-a-bug for an example walk through of how to do this

@gundalow
Copy link
Contributor

Please add the following into the DOCUMENTATION block after line 118

    force_deletion:
        description:
          - Forcabily delete the service. Required when deleting a service with >0 scale, or no target group.
        default: False
        type: bool

@asaf400
Copy link
Contributor Author

asaf400 commented Nov 16, 2020 •
edited
Loading

I think I still have a bit of work on 'tests/integration/targets/ecs_cluster/tasks/force_service_deletion.yml'
I'm not sure if I'm done with that file, I'll try to make sure it looks okay during this week..

Edit: Also, Done pushing documentation block commit

@ansibullbot
Copy link

@asaf400 This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI stale_ci CI is older than 7 days, rerun before merging labels Nov 19, 2020
@ansibullbot
Copy link

cc @markuman
click here for bot help

@ansibullbot ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 22, 2021
@ansibullbot ansibullbot added community_review and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 22, 2021
@tremble
Copy link
Contributor

tremble commented Oct 22, 2021

rebased, tested locally and added some missing documentation (changelog / version_added)

@tremble
Copy link
Contributor

tremble commented Oct 22, 2021

I'm sorry it's taken so long to get this merged. I've fixed up the tests and added some minor documentation but I think this is now ready to merge.

@tremble tremble added the gate label Oct 22, 2021
ansible-zuul[bot]
ansible-zuul bot approved these changes Oct 22, 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!

@ansible-zuul ansible-zuul bot merged commit 458177c into ansible-collections:main Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ansible-zuul Ansible Zuul Ansible Zuul approved these changes

Assignees
No one assigned
Labels
affects_2.10 community_review integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@asaf400 @ansibullbot @gundalow @tremble