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

scaleway: Fix case mismatch causing key lookup failure #68295

Closed
wants to merge 1 commit into from

Conversation

olof
Copy link

@olof olof commented Mar 18, 2020

SUMMARY

Since commit f70dc26, automatic jsonification only happens if the Content-Type header is application/json, but the class only included Content-type in its default set of headers (notice the case variation).

This caused a KeyError on send() if the caller relied on the default content-type value.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ansible.module_utils.scaleway

ADDITIONAL INFORMATION
  • I noticed this with the scaleway_compute module, but as it's shared widely among all the scaleway modules, I think more are affected.
  • The commit f70dc26 has not been part of any ansible releases. (So, I omitted a changelog fragment, as I think that superfluous? Correct me if I'm wrong.)

Since commit f70dc26, the common http api client class used by the
scaleway modules only enables automatic jsonification of the request
body if the "Content-Type" header is application/json. The client only
included "Content-type" in its default set of headers (notice the case
variation).

This caused a KeyError on send() if the caller relied on the default
content-type value.
@ansibot
Copy link
Contributor

ansibot commented Mar 18, 2020

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. cloud 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. new_contributor This PR is the first contribution by a new community member. scaleway small_patch support:community This issue/PR relates to code supported by the Ansible community. labels Mar 18, 2020
@remyleone
Copy link
Contributor

/shipit

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 22, 2020
@olof
Copy link
Author

olof commented Mar 26, 2020

I notice that now that my pull request comes in the midst of a split of the ansible repo. Are you planning on creating an ansible-collections repo for the scaleway modules? If so, will you rebase this pull request or should I reopen once you've migrated? Same goes for the other pull request I opened. I have some other possible changes on the way, but I'll hold off on those a bit. Thanks! :)

@remyleone
Copy link
Contributor

Hi Olof, thanks a lot for your contribution :) At the moment, we are focusing much of our effort on the CLI and the terraform provider for Scaleway. Once those projects are feature-complete we will come back to add support for all new products (Kapsule, LoadBalancer, Baremetal, Database, ...) inside ansible.

As for the collections, we are interested because it will allow us to have our own schedule for releasing new versions without waiting for ansible maintainers approval. At the moment we haven’t investigated how to migrate our existing modules to ansible collections but we will do so soon.

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 26, 2020
@olof
Copy link
Author

olof commented Mar 26, 2020

So, just to confirm: I'll just wait until you've set that up :).

(The scaleway modules have been removed from this repo, so I will not be able to resolve the needs_rebase label.)

@ansibot ansibot added collection Related to Ansible Collections work collection:community.general collection:sh4d1.scaleway needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md labels Apr 24, 2020
@gundalow
Copy link
Contributor

gundalow commented Jun 2, 2020

@remyleone Hi, the scaleway modules have been migrated to community.general https://github.com/ansible-collections/community.general If you've like to move these to their own collection please follow this procedure https://github.com/ansible-collections/overview/blob/master/README.rst#q-what-should-i-do-to-move-plugins-across-collections-during-migration It would be nice to see this plugins moved under https://github.com/scaleway/ as you say, that means you can release at your own schedule. Please do shout out in #ansible-community on Freenode IRC if we can help with this

@olof Hi, could you please create a new PR against https://github.com/ansible-collections/community.general/blob/master/plugins/module_utils/scaleway.py then we can get it merged. If a new collection does get created, it will be done by migrating from community.general.

@olof
Copy link
Author

olof commented Jun 2, 2020

@gundalow thanks, I've opened a new pull request at ansible-collections/community.general#445.

@olof olof closed this Jun 2, 2020
@ansible ansible locked and limited conversation to collaborators Jun 30, 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 bug This issue/PR relates to a bug. cloud collection:community.general collection Related to Ansible Collections work needs_collection_redirect https://github.com/ansible/ansibullbot/blob/master/docs/collection_migration.md needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. scaleway small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. 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