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

Fix invalid API request to Rackspace Cloud DNS - Issue #981 #989

Merged
merged 2 commits into from
Oct 15, 2021
Merged

Fix invalid API request to Rackspace Cloud DNS - Issue #981 #989

merged 2 commits into from
Oct 15, 2021

Conversation

mattgauf
Copy link
Contributor

Fix for Issue #981 invalid API request to Rackspace Cloud DNS.

Rackspace's API returns 403 for GET requests that include a request body. After reviewing my original solution (mentioned in the linked issue), I determined that a better solution was possible.

My decision-making process was:

  1. Keep the assignment of data to an empty literal at the start of the method for consistency with expected bahvior.
  2. Keep json.dumps() inline at the data argument to ensure that all requests that send a JSON payload are parsed.
  3. Provide an inline expression that returns the JSON string unless the action is GET, else return None. (Requests library omits request body when the value is None type.)
  4. Add comment before request indicating that the inline expression is intended to address an API issue.

I also added myself to the CODEOWNERS file after reviewing the developer guide. If I missunderstood the purpose of CODEOWNERS then please feel free to remove me.

@mattgauf
Copy link
Contributor Author

@AnalogJ This is my first time contributing to this repository, please let me know if there are any guidelines I missed, or changes I need to make.

Best,
-- M

@mattgauf
Copy link
Contributor Author

@adferrand I'm mentioning you in the PR since it appears you've been more active recently.

Right now the Rackspace provider is broken in the current release. If you have time, please look over this PR and tell me if there is anything I need to update or change.

Best,
-- M

@adferrand
Copy link
Collaborator

No I have everything needed. You fixed the issue and the integration tests still work, proving that the fix has no known side effect. I merge and prepare a new release.

Thanks a lot!

@adferrand adferrand merged commit 4ca3148 into AnalogJ:master Oct 15, 2021
@mattgauf mattgauf deleted the bugfix/issue-981-invalid-api-request-to-rackspace branch October 15, 2021 16:09
MasinAD pushed a commit to MasinAD/lexicon that referenced this pull request Mar 29, 2022
…nalogJ#989)

* Solution for Issue AnalogJ#981. Update _request method for Rackspace Cloud

* Add @mattgauf to CODEOWNERS for Rackspace
MasinAD pushed a commit to MasinAD/lexicon that referenced this pull request Mar 29, 2022
…nalogJ#989)

* Solution for Issue AnalogJ#981. Update _request method for Rackspace Cloud

* Add @mattgauf to CODEOWNERS for Rackspace
MasinAD pushed a commit to MasinAD/lexicon that referenced this pull request Mar 29, 2022
…nalogJ#989)

* Solution for Issue AnalogJ#981. Update _request method for Rackspace Cloud

* Add @mattgauf to CODEOWNERS for Rackspace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants