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

start/stop/restart azure webapp #44498

Merged
merged 8 commits into from
Aug 28, 2018
Merged

start/stop/restart azure webapp #44498

merged 8 commits into from
Aug 28, 2018

Conversation

yungezz
Copy link
Contributor

@yungezz yungezz commented Aug 22, 2018

SUMMARY

support start/stop/restart webapp.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
ANSIBLE VERSION
ansible 2.7
ADDITIONAL INFORMATION

@yungezz yungezz requested a review from zikalino August 22, 2018 09:38
@yungezz
Copy link
Contributor Author

yungezz commented Aug 22, 2018

@yuwzho

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 azure cloud community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Aug 22, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 22, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_webapp.py:986:42: undefined-variable Undefined variable 'ex_request_id'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_webapp.py:713:20: E125 continuation line with same indent as next logical line

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/azure/azure_rm_webapp.py:0:0: E324 Value for "default" from the argument_spec ('start') for "power_action" does not match the documentation (None)

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. community_review In order to be merged, this PR must follow the community review workflow. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 22, 2018
@ansibleazurebot ansibleazurebot mentioned this pull request Aug 23, 2018
Closed
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Aug 23, 2018
zikalino
zikalino reviewed Aug 27, 2018
View reviewed changes
lib/ansible/modules/cloud/azure/azure_rm_webapp.py Outdated
@@ -157,6 +157,16 @@
- Purge any existing application settings. Replace web app application settings with app_settings.
type: bool

power_action:
Copy link
Contributor

Choose a reason for hiding this comment

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

just wonder if power_action is a proper name for this, because it's really a state.
I was just thinking about virtual machines, and a kind of I don't like the interface there either (started / restarted / allocated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to app_state per offline sync

zikalino
zikalino reviewed Aug 27, 2018
View reviewed changes
lib/ansible/modules/cloud/azure/azure_rm_webapp.py Outdated
@@ -932,6 +967,29 @@ def get_webapp_configuration(self):

return False

def start_webapp(self, action):
Copy link
Contributor

Choose a reason for hiding this comment

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

this name is a bit misleading, because it can be also used to stop the application.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe set_application_state would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to change_webapp_state

@ansibot
Copy link
Contributor

ansibot commented Aug 27, 2018

cc @haroldwongms @nitzmahone @trstringer @xscript @zikalino
click here for bot help

@zikalino
Copy link
Contributor

in general everything looks good, i just have some doubts regarding option name. "power action" more applies to hardware devices, like virtual machines, shoudln't it be just "action", or application_state? let me think a bit more.

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

@yungezz this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 28, 2018
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed merge_commit This PR contains at least one merge commit. Please resolve! labels Aug 28, 2018
@Fred-sun
Copy link
Contributor

@yungezz This PR has some conflicting files need you help to resolve? Thanks!

@yungezz
Copy link
Contributor Author

yungezz commented Aug 28, 2018

working on it

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 28, 2018
@yungezz
Copy link
Contributor Author

yungezz commented Aug 28, 2018

ready_for_review

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 28, 2018
zikalino
zikalino reviewed Aug 28, 2018
View reviewed changes
lib/ansible/modules/cloud/azure/azure_rm_webapp.py Outdated
@@ -410,6 +420,11 @@ def __init__(self):
type='bool',
default=False
),
app_state=dict(
type='str',
choices=['start', 'stop', 'restart'],
Copy link
Contributor

Choose a reason for hiding this comment

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

hehe, still wondering, would be better to have "started", "stopped", "restarted"?

Copy link
Contributor

Choose a reason for hiding this comment

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

or, "running", "stopped", "restart"?

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 28, 2018
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 28, 2018
@zikalino zikalino merged commit 2bd0a66 into ansible:devel Aug 28, 2018
@zikalino zikalino deleted the yungez-webapp-start branch August 28, 2018 23:41
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zikalino zikalino zikalino approved these changes

Assignees
No one assigned
Labels
affects_2.7 This issue/PR affects Ansible v2.7 azure cloud community_review In order to be merged, this PR must follow the community review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yungezz @ansibot @zikalino @Fred-sun @jborean93