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

adding container registry facts #43325

Merged
merged 26 commits into from
Aug 30, 2018
Merged

Conversation

zikalino
Copy link
Contributor

SUMMARY

Creating missing container registry facts.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

azure_rm_containerregistry_facts

ANSIBLE VERSION

2.6

ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Jul 26, 2018

Hi @zikalino,

Thank you for the pullrequest, 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

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.7 This issue/PR affects Ansible v2.7 azure cloud module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Jul 26, 2018
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Jul 27, 2018
@ansibot 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 Aug 4, 2018
@Fred-sun
Copy link
Contributor

@zikalino Could you help to confirm the PR ready for review or not? Thanks!

1 similar comment
@Fred-sun
Copy link
Contributor

@zikalino Could you help to confirm the PR ready for review or not? Thanks!

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

@zikalino
Copy link
Contributor Author

@Fred-sun this PR has label WIP which means that it's not ready for review

@Fred-sun
Copy link
Contributor

@zikalino I'm sorry, The PR have no update more than one week, So I update the comment to understand the PR status. Thanks!

@ansibot 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 Aug 29, 2018
description: A list of dict results where the key is the name of the Registry and the values are the facts for that Registry.
returned: always
type: complex
contains:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we show username/password, webhook list, registry list, registry tag list and buildtask list in the facts?

@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:197:1: E305 expected 2 blank lines after class or function definition, found 0

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Aug 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

cc @yaweiw
click here for bot help

@ansibot ansibot added test This PR relates to tests. and removed ci_verified Changes made in this PR are causing tests to fail. labels Aug 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:197:1: E305 expected 2 blank lines after class or function definition, found 0

click here for bot help

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Aug 29, 2018
pass


class AzureRMRegistryFacts(AzureRMModuleBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

AzureRMContainerRegistryFacts

@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 29, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:224:120: E241 multiple spaces after ','
lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:224:161: E501 line too long (219 > 160 characters)

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:228:24: E201 whitespace after '{'
lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:230:120: E241 multiple spaces after ','
lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:230:161: E501 line too long (219 > 160 characters)
lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:244:26: E203 whitespace before ':'

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Aug 29, 2018

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

lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:228:24: E201 whitespace after '{'
lib/ansible/modules/cloud/azure/azure_rm_containerregistry_facts.py:244:26: E203 whitespace before ':'

click here for bot help

@zikalino
Copy link
Contributor Author

@yungezz returning credentials follows azure_rm_containerregistry

@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 29, 2018
credentials:
description:
- Credentials, fields will be empty if admin user is not enabled for ACR
return: always
Copy link
Contributor

Choose a reason for hiding this comment

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

return is not always, but when retrieve option is on

returned: when registry exists and C(admin_user_enabled) is set
type: str
sample: zim
password:
Copy link
Contributor

Choose a reason for hiding this comment

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

jordan mentioned to add no_log to returned value this morning. do you know how to add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, we can search... but I guess I we have option to specifically enable passwords, we can skip it for now

Copy link
Contributor

Choose a reason for hiding this comment

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

don't think so. the option is weak, just make it little difficult to get credential. no_log is really make it not available in another other place other than ansible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yungezz no_log can be placed in the playbook, we do not have to do anything in the module itself

credentials:
description:
- Credentials, fields will be empty if admin user is not enabled for ACR
return: when C(retrieve_credentials) is set and C(admin_user_enabled) is set on ACR
Copy link
Contributor

Choose a reason for hiding this comment

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

returned :( will lint fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will recheck this once again

@yungezz yungezz merged commit bd866ed into ansible:devel Aug 30, 2018
@yungezz yungezz deleted the facts-containerregistry branch August 30, 2018 09:56
@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.
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. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants