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.

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_info_tags into the doc_fragments #55548

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@MyronFanQiu
Copy link
Contributor

commented Apr 19, 2019

use azure_info_tags in the azure_rm_containerregistry_facts

SUMMARY

Add a new docfragment azure_info_tags
Modify the tags part of the documentation of the azure_rm_containerregistry_facts module
Supports setting supports_tags=True in the info modules

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

azure_info_tags
azure_rm_containerregistry_facts

ADDITIONAL INFORMATION

add azure_info_tags into the doc_fragments
use azure_info_tags in the azure_rm_containerregistry_facts as a try
@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@MyronFanQiu, 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

@MyronFanQiu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@zikalino Hi, Zim! I opened this PR based on our discussion. But I'm not pretty familiar with the doc fragment part and I may miss something. Please help me check this PR when you are free. Thanks!

@zikalino

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@MyronFanQiu looks good, we could also add support for support_tags=True for _info modules in the same PR?

@ansibot ansibot removed the needs_triage label Apr 19, 2019

@zikalino

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

we could have sth similar in azure_rm_common:

AZURE_TAG_ARGS = dict(
tags=dict(type='dict'),
append_tags=dict(type='bool', default=True),
)

--->

AZURE_TAG_ARGS_INFO = dict(
tags=dict(type='list')
)

@MyronFanQiu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@zikalino ok~ Let me have a try.

@MyronFanQiu MyronFanQiu force-pushed the VSChina:fanqiu-facts-tags-fix branch from 6c2fef8 to 152493a Apr 19, 2019

@MyronFanQiu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

@zikalino It supports supports_tags=True in the info modules now, and I add a weak restriction onto the function validate_tags for info modules. If you have any comment or suggestion, please let me know. Thanks!

@Fred-sun

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

@zikalino Please review this PR when you're available! Thanks!

@MyronFanQiu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

@yuwzho @yungezz Hi! When you have time, please take a look at this PR~

@MyronFanQiu MyronFanQiu changed the title [WIP] add azure_info_tags into the doc_fragments Add azure_info_tags into the doc_fragments Apr 25, 2019

@ansibot ansibot added community_review and removed WIP labels Apr 25, 2019

@ansibot ansibot added the stale_ci label May 3, 2019

@Fred-sun

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@yungezz @yuwzho Please help review this PR when you're available! Thanks!

@@ -379,6 +386,9 @@ def validate_tags(self, tags):
for key, value in tags.items():
if not isinstance(value, str):
self.fail("Tags values must be strings. Found {0}:{1}".format(str(key), str(value)))
else:
if not isinstance(tags, list) and not isinstance(tags, dict):
self.fail("Tags must be a dictionary of string:string values or list of string values in info modules")

This comment has been minimized.

Copy link
@yuwzho

yuwzho May 5, 2019

Contributor

does it need to check value when the tag is dict?

This comment has been minimized.

Copy link
@MyronFanQiu

MyronFanQiu May 5, 2019

Author Contributor

From my view, the ideal solution is to make sure that tags is the list type in the facts module. However, there are already many facts modules and I'm not sure that all of them supports the list type. A soft check(supports list and dict) may avoid possible errors. For the new module, if we use AZURE_INFO_TAG_ARGS, the type of tags will be limited to list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.