-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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.
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
add azure_rm_webapp_facts module #43631
Conversation
Hi @yungezz, Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for azure. |
The test
The test
The test
The test
|
@zikalino The PR has ready for revew. Could you help to take a look? Thanks! |
f357426
to
6dccc71
Compare
The test
The test
The test
The test
|
@yungezz Please help confirm, does this PR ready for review? |
returned: always | ||
type: list | ||
example: [{ | ||
"app_settings": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return value should be documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
self.results['ansible_facts']['azure_webapps'][0]['app_settings'] = self.list_webapp_appsettings() | ||
|
||
elif self.resource_group: | ||
self.results['ansible_facts']['azure_webapps'] = self.list_by_resource_group() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recently the idea is not to place facts inside 'ansible_facts'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then where to put it? has the idea been agreed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, even mentioned today on the meeting...
also I would rename 'azure_webapps' to just 'webapps', as azure part is a kind of obvious
The test
The test
The test
The test
|
- name: net_framework | ||
version: v4.0 | ||
- name: node | ||
versoin: '6.6' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
results = [] | ||
for item in response: | ||
if self.has_tags(item.tags, self.tags): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there could be a function to convert entire list, as this code is repeated below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} | ||
curated_output['frameworks'].append(fx) | ||
|
||
# java container setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
description: | ||
- Other useful properties of the web app, which is not curated to module input. | ||
return: always | ||
type: complex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also should be described
@brusMX @devigned @gustavomcarmo @haroldwongms @julienstroheker @lmazuel @obsoleted @sozercan @tripdubroot @trstringer @tstringer @xscript @yaweiw @yuwzho As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
lastModifiedTimeUtc: | ||
description: Last modified date of the web app. | ||
type: str | ||
outboundIpAddresses: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is one or more addresses?
@yungezz Could you help update the comment? Thanks! |
The test
|
SUMMARY
add azure_rm_webapp_facts module
ISSUE TYPE
COMPONENT NAME
azure_rm_webapp_facts
ANSIBLE VERSION
ADDITIONAL INFORMATION