Added ability to specify plan information for images that require it. #65335

seanw122 · 2019-11-27T18:41:09Z

SUMMARY

Added ability to specify billing plan information for images that require it. Images like CIS hardened images on Azure marketplace require plan field to be populated.
Fixes #65268

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

azure_rm_virtualmachinescaleset

ADDITIONAL INFORMATION

Tests were done locally that:

generated the VM scale set with CIS image
generated VM scale set with standard Ubuntu image to prove nothing broke
as expected, failed to generate a scale set when specifying plan for image that does not require it
as expected, failed to generate a scale set when specifying image without required plan

The module azure_rm_virtualmachinescaleset handles plan information. Based on how plan and accept_terms was used I copied:

over documentation
over parameter definition
over code blocks for validating plan parameter
over code for accepting terms
and
added an example in the examples section.

ansibot · 2019-11-27T18:42:47Z

cc @haroldwongms @nitzmahone @sozercan @yungezz @yuwzho @zikalino
click here for bot help

ansibot · 2019-11-27T18:42:48Z

@seanw122, just so you are aware we have a dedicated Working Group for azure.
You can find other people interested in this in #ansible-azure on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py

felixfontein · 2019-11-27T19:49:34Z

lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py

+                    # Before creating VM accept terms of plan if `accept_terms` is True
+                    if self.accept_terms is True:
+                        if not self.plan or not all([self.plan.get('name'), self.plan.get('product'), self.plan.get('publisher')]):
+                            self.fail("parameter error: plan must be specified and include name, product, and publisher")


You can use required_if to make sure that plan must be specified if accept_terms is True.

I think this needs to be the other way around. If a plan is specified then accept_terms must be True. I left it in for now until I learn the syntax for required_if.

Your logic from the code above is: if accept_terms is True, plan must be specified. (That can also be modelled by required_if.) If you want the logic the other way around, you have to change your code (and you can't use required_if AFAIK).

changelogs/fragments/65335-add-plan-to-azure-vmscaleset-module.yaml

Fred-sun · 2019-11-28T08:42:47Z

review_need

ansibot · 2019-11-28T17:08:14Z

@seanw122 this PR contains the following merge commits:

0083cac

Please rebase your branch to remove these commits.

click here for bot help

… plan since suboptions were added.

….yaml Co-Authored-By: Felix Fontein <felix@fontein.de>

ansibot · 2019-11-28T19:04:48Z

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

lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py:518:111: trailing-whitespace: Trailing whitespace

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

lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py:518:34: E128: continuation line under-indented for visual indent
lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py:518:112: W291: trailing whitespace
lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py:519:34: E126: continuation line over-indented for hanging indent

click here for bot help

ansibot · 2019-11-29T02:23:31Z

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

lib/ansible/modules/cloud/azure/azure_rm_virtualmachinescaleset.py:518:17: E128: continuation line under-indented for visual indent

click here for bot help

… due to code.

seanw122 · 2019-11-29T16:41:59Z

In fairness, this is a feature change and not a bug. I don't know how to remove bug tag.

felixfontein · 2019-11-29T17:13:30Z

@seanw122 you can simply edit the PR message and change Bugfix Pull Request to Feature Pull Request. ansibot should then update the labels accordingly.

seanw122 · 2019-11-29T17:33:17Z

Hmm. Well it added Feature but did not remove Bug. I don't know if the bot will let me execute notabug.

seanw122 · 2019-11-29T18:08:50Z

Thanks @felixfontein !

anshulbehl · 2019-12-03T20:43:00Z

@gavinfish Can you review this and I think this should also be contributed to the collection as well.

Fred-sun · 2019-12-05T05:25:28Z

ready_for_review

haiyuazhang · 2019-12-11T09:37:52Z

@seanw122
I just checked the azure doc, seems that "the accept term" is an one-shot endeavor, so IMO, maybe we can remove that part from this PR. Besides , if full automation is the ultimate goal, should we move the accept term logic to azure_rm_image?

I'll invite one of the committer to review your PR, and handle it as soon as possible. Thanks for contributing to ansible azure module.

seanw122 · 2019-12-11T22:13:00Z

@haiyuazhang I removed the property and related code for accepting terms.

felixfontein reviewed Nov 27, 2019

View reviewed changes

ansibot removed the needs_triage Needs a first human triage before being processed. label Nov 27, 2019

felixfontein reviewed Nov 27, 2019

View reviewed changes

changelogs/fragments/65335-add-plan-to-azure-vmscaleset-module.yaml Outdated Show resolved Hide resolved

ansibleazurebot mentioned this pull request Nov 28, 2019

Added ability to specify plan information for images that require it. VSChina/ansible-issue-tracker#1208

Closed

ansibot added the has_issue label Nov 28, 2019

Sean Whitesell and others added 5 commits November 28, 2019 11:21

Modified to include plan information.

02e5603

Fixed where plan is passed to the virtual machine object.

a7cbcb3

Added changelog file

4136fd6

Added plan suboptions with required flag. Removed code block checking…

1e0e146

… plan since suboptions were added.

Update changelogs/fragments/65335-add-plan-to-azure-vmscaleset-module…

ae9c601

….yaml Co-Authored-By: Felix Fontein <felix@fontein.de>

seanw122 force-pushed the devel branch from 0083cac to ae9c601 Compare November 28, 2019 17:23

ansibot removed 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 labels Nov 28, 2019

Removed extra blank line. Added promotion_code to plan param list.

8311052

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 28, 2019

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Nov 28, 2019

Trying to fix indention issue

948960c

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 29, 2019

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Nov 29, 2019

Trying to fix indention

a4daf05

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 29, 2019

Changed example capacity to trigger build check. Last failure was not…

97ced6e

… due to code.

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 Nov 29, 2019

ansibot added the feature This issue/PR relates to a feature request. label Nov 29, 2019

felixfontein removed the bug This issue/PR relates to a bug. label Nov 29, 2019

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 11, 2019

Removed property for accepting terms and code block using it..

5a37118

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 11, 2019

Removed extra unneeded spaces.

58bae51

zikalino approved these changes Dec 18, 2019

View reviewed changes

zikalino merged commit dfd998b into ansible:devel Dec 18, 2019

ansible locked and limited conversation to collaborators Jan 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to specify plan information for images that require it. #65335

Added ability to specify plan information for images that require it. #65335

seanw122 commented Nov 27, 2019 •

edited

ansibot commented Nov 27, 2019

ansibot commented Nov 27, 2019

felixfontein Nov 27, 2019

seanw122 Nov 28, 2019

felixfontein Nov 28, 2019

Fred-sun commented Nov 28, 2019

ansibot commented Nov 28, 2019

ansibot commented Nov 28, 2019

ansibot commented Nov 29, 2019

seanw122 commented Nov 29, 2019

felixfontein commented Nov 29, 2019

seanw122 commented Nov 29, 2019

seanw122 commented Nov 29, 2019

anshulbehl commented Dec 3, 2019

Fred-sun commented Dec 5, 2019

haiyuazhang commented Dec 11, 2019

seanw122 commented Dec 11, 2019

Added ability to specify plan information for images that require it. #65335

Added ability to specify plan information for images that require it. #65335

Conversation

seanw122 commented Nov 27, 2019 • edited

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibot commented Nov 27, 2019

ansibot commented Nov 27, 2019

felixfontein Nov 27, 2019

Choose a reason for hiding this comment

seanw122 Nov 28, 2019

Choose a reason for hiding this comment

felixfontein Nov 28, 2019

Choose a reason for hiding this comment

Fred-sun commented Nov 28, 2019

ansibot commented Nov 28, 2019

ansibot commented Nov 28, 2019

ansibot commented Nov 29, 2019

seanw122 commented Nov 29, 2019

felixfontein commented Nov 29, 2019

seanw122 commented Nov 29, 2019

seanw122 commented Nov 29, 2019

anshulbehl commented Dec 3, 2019

Fred-sun commented Dec 5, 2019

haiyuazhang commented Dec 11, 2019

seanw122 commented Dec 11, 2019

seanw122 commented Nov 27, 2019 •

edited