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

Moved from Accept to Accept-Encoding header in secrets batch endpoint #2065

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

telday
Copy link
Contributor

@telday telday commented Mar 11, 2021

What does this PR do?

When I first implemented this fix I chose to have the batch secrets retrieval endpoint check the Accept Header, however after reading the RFC relating to the two headers and putting some thought into it I believe the header should be changed.

I wanted to address this issue because currently it will be easy to update the integrations which use this header (only the Go client and OpenAPI spec), however left alone it will become an annoying piece of technical debt.

Checklists

Change log

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
  • This PR does not require updating any documentation

API Changes

  • The OpenAPI spec has been updated to meet new API changes (or an issue has been opened), or
  • The changes in this PR do not affect the Conjur API

Related OpenAPI PR

@telday telday requested a review from a team as a code owner March 11, 2021 19:52
@codeclimate
Copy link

codeclimate bot commented Mar 11, 2021

Code Climate has analyzed commit 61fc14bc and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 89.4% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@micahlee micahlee left a comment

Choose a reason for hiding this comment

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

Yeah, this makes sense to me.

Do you mind updating the response to also include the Content-Encoding header?

@telday
Copy link
Contributor Author

telday commented Mar 11, 2021

@micahlee Updated the response headers and added a new check in the tests to ensure the header gets set

@telday telday requested a review from micahlee March 11, 2021 20:28
CHANGELOG.md Show resolved Hide resolved
The secrets batch endpoint used to look at the Accept
header to determine if it should base64-encode the response.
Now it looks at the Accept-Encoding header. This change
falls in line better with the intended uses of both
headers
Copy link
Contributor

@micahlee micahlee left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

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

Successfully merging this pull request may close these issues.

2 participants