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 new aws module - iam_cert_facts #24451

Merged
merged 1 commit into from May 16, 2017

Conversation

Projects
None yet
6 participants
@Lujeni
Contributor

Lujeni commented May 10, 2017

SUMMARY

Add new aws module.
Retrieves iam_cert details using AWS methods.

ISSUE TYPE
  • Feature Pull Request
  • New Module Pull Request
ADDITIONAL INFORMATION

related to #19685

@ansibot

This comment has been minimized.

Show comment
Hide comment
@ansibot

ansibot May 10, 2017

Contributor

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/amazon/iam_cert_facts.py:0:0: E319 RETURN.ResponseMetadata.description: expected list for dictionary value @ data['description']. Got None
lib/ansible/modules/cloud/amazon/iam_cert_facts.py:0:0: E319 RETURN.ServerCertificateMetadataList.description: expected list for dictionary value @ data['description']. Got None

click here for bot help

Contributor

ansibot commented May 10, 2017

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/cloud/amazon/iam_cert_facts.py:0:0: E319 RETURN.ResponseMetadata.description: expected list for dictionary value @ data['description']. Got None
lib/ansible/modules/cloud/amazon/iam_cert_facts.py:0:0: E319 RETURN.ServerCertificateMetadataList.description: expected list for dictionary value @ data['description']. Got None

click here for bot help

Show outdated Hide outdated test/sanity/pep8/legacy-files.txt Outdated

@gundalow gundalow removed the needs_triage label May 10, 2017

@ansibot ansibot removed the ci_verified label May 11, 2017

@ansibot

This comment has been minimized.

Show comment
Hide comment
@ansibot

ansibot May 11, 2017

Contributor

The test ansible-test sanity --test pep8 failed with the following error:

test/sanity/pep8/legacy-files.txt:161:1: A201 Remove "lib/ansible/modules/cloud/amazon/iam_cert_facts.py" since it passes the current rule set

click here for bot help

Contributor

ansibot commented May 11, 2017

The test ansible-test sanity --test pep8 failed with the following error:

test/sanity/pep8/legacy-files.txt:161:1: A201 Remove "lib/ansible/modules/cloud/amazon/iam_cert_facts.py" since it passes the current rule set

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels May 11, 2017

@Lujeni

This comment has been minimized.

Show comment
Hide comment
@Lujeni

Lujeni May 11, 2017

Contributor

@gundalow imho, all is good now.

Contributor

Lujeni commented May 11, 2017

@gundalow imho, all is good now.

@gundalow gundalow requested review from ryansb and s-hertel May 11, 2017

@gundalow

This comment has been minimized.

Show comment
Hide comment
@gundalow

gundalow May 11, 2017

Contributor

Thanks for fixing that. I'll hand over to someone that knows AWS more for the final review.

Contributor

gundalow commented May 11, 2017

Thanks for fixing that. I'll hand over to someone that knows AWS more for the final review.

results = get_server_certificate(
client=iam_cert, module=module, server_certificate_name=certificate_name)
else:
results = list_server_certificates(client=iam_cert, module=module, path_prefix=path_prefix)

This comment has been minimized.

@ryansb

ryansb May 12, 2017

Member

Please use the camel_dict_to_snake_dict to fix the camelcase in the boto3 responses here.

@ryansb

ryansb May 12, 2017

Member

Please use the camel_dict_to_snake_dict to fix the camelcase in the boto3 responses here.

Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
@Lujeni

This comment has been minimized.

Show comment
Hide comment
@Lujeni

Lujeni May 12, 2017

Contributor

Thanks @ryansb for the review.
All changes are made.

The diff https://pastebin.com/6sZcMNwh (I have rebase my "review" commit).

Contributor

Lujeni commented May 12, 2017

Thanks @ryansb for the review.
All changes are made.

The diff https://pastebin.com/6sZcMNwh (I have rebase my "review" commit).

Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
Show outdated Hide outdated lib/ansible/modules/cloud/amazon/iam_cert_facts.py Outdated
@Lujeni

This comment has been minimized.

Show comment
Hide comment
@Lujeni

Lujeni May 15, 2017

Contributor

@ryansb @s-hertel Thanks again for the review

I change and rebase the code.

If you prefer to check the diff before the rebase:

review2

Contributor

Lujeni commented May 15, 2017

@ryansb @s-hertel Thanks again for the review

I change and rebase the code.

If you prefer to check the diff before the rebase:

review2

@Lujeni

This comment has been minimized.

Show comment
Hide comment
@Lujeni

Lujeni May 15, 2017

Contributor

BTW, there is a simple way to test this kind of module ?

Thanks

Contributor

Lujeni commented May 15, 2017

BTW, there is a simple way to test this kind of module ?

Thanks

@s-hertel

This comment has been minimized.

Show comment
Hide comment
@s-hertel

s-hertel May 15, 2017

Contributor

@Lujeni Documenting well what is returned and when things are returned is usually sufficient for facts modules.

Contributor

s-hertel commented May 15, 2017

@Lujeni Documenting well what is returned and when things are returned is usually sufficient for facts modules.

@ryansb

ryansb approved these changes May 16, 2017

@ryansb

This comment has been minimized.

Show comment
Hide comment
@ryansb

ryansb May 16, 2017

Member

shipit

Member

ryansb commented May 16, 2017

shipit

@s-hertel s-hertel merged commit a3a0742 into ansible:devel May 16, 2017

1 check passed

Shippable Run 21783 status is SUCCESS.
Details
@tedder

This comment has been minimized.

Show comment
Hide comment
@tedder

tedder May 16, 2017

Contributor

@Lujeni In the future, please include code as text, not as screenshots. Here's how.

Contributor

tedder commented May 16, 2017

@Lujeni In the future, please include code as text, not as screenshots. Here's how.

KKoukiou added a commit to KKoukiou/ansible that referenced this pull request May 22, 2017

@s-hertel

This comment has been minimized.

Show comment
Hide comment
@s-hertel

s-hertel Jun 5, 2017

Contributor

Hi @Lujeni

When reviewing we didn't see that this was a duplicate of the iam_server_certificate_facts module. I'm so sorry it wasn't caught and that you spent time and effort on it. Since this hasn't been in a stable release it can be removed easily and I've made a PR to do that here.

Contributor

s-hertel commented Jun 5, 2017

Hi @Lujeni

When reviewing we didn't see that this was a duplicate of the iam_server_certificate_facts module. I'm so sorry it wasn't caught and that you spent time and effort on it. Since this hasn't been in a stable release it can be removed easily and I've made a PR to do that here.

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment