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

Revert "Make cloudfront_info work with Python3 …" #67085

Merged
merged 1 commit into from Feb 10, 2020

Conversation

stefanhorning
Copy link
Contributor

Reverts #66695

After testing again today I realized my change caused issues. Changing it back solved the issue. Also I couldn't reproduce the initial issue anymore I was trying to fix with this change. As it appears distributions is always a list and never a dict. So don't know how I could provoke the error described in #66695 (where merging dicts was the issue).

So I think it should be reverted.

@stefanhorning
Copy link
Contributor Author

@tremble maybe you can confirm this...

@ansibot
Copy link
Contributor

ansibot commented Feb 4, 2020

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 aws cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. python3 support:community This issue/PR relates to code supported by the Ansible community. labels Feb 4, 2020
@@ -504,7 +504,7 @@ def get_distribution_id_from_domain_name(self, domain_name):
try:
distribution_id = ""
distributions = self.list_distributions(False)
distributions.update(self.list_streaming_distributions(False))
distributions += self.list_streaming_distributions(False)

Choose a reason for hiding this comment

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

distributions.extend(self.list_streaming_distributions(False))
+= may be fast , extend bring clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but both work for lists only, right?
The question remains how I managed to run into the initial error described here #66695 (comment) as I can't reproduce it anymore. The problem was that somehow both elements were dicts in that case. But now the update won't work anymore as in fact both elements are lists now, but I am still on python 3.7.

Choose a reason for hiding this comment

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

list_streaming_distributions(self, keyed=True) function seems to be returning dict. Shouldn't returning list be the erorr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure at this point. I can do more testing if you want. All I figured out yesterday was that the cloudfron_info was broken for me so I reverted my initial "fix" which resolved the issue.

Choose a reason for hiding this comment

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

If you can share how are you testing it, I can also give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use

- cloudfront_info:
    domain_name_alias: 'mydomain.alias.com'
    distribution_config: yes
    region: us-east-1

which doesn't work with distributions.update() fix being present.

@stefanhorning
Copy link
Contributor Author

stefanhorning commented Feb 10, 2020

After debugging a bit more and looking through all playbook I have in my repo that use the clodufront_info module I could not find a way to make the module work with distributions.update() being present. It fails every time.
Hence we should really merged this PR, yo make sure my change from #66695 doesn't make it into a release, where it most definitely will break all the playbooks that use this module!

cc @jillr @s-hertel @wilvk @tremble @Himanshu54

@tremble
Copy link
Contributor

tremble commented Feb 10, 2020

LGTM

@ansibot ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. needs_triage Needs a first human triage before being processed. labels Feb 10, 2020
@ansibot ansibot merged commit fa8c07a into ansible:devel Feb 10, 2020
href pushed a commit to cloudscale-ch/ansible that referenced this pull request Feb 12, 2020
@ansible ansible locked and limited conversation to collaborators Mar 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 automerge This PR was automatically merged by ansibot. aws cloud module This issue/PR relates to a module. python3 shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants