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

Handle proprietary headers in spec summary. #57

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mgritter
Copy link
Contributor

@mgritter mgritter commented Jul 9, 2021

First of the downstream changes handling the IR change in postmanlabs/observability-ir#2.

Probably they need to go in together; I'll do the CLI changes next.

@mgritter mgritter requested review from liujed and thatplguy July 9, 2021 23:31
Copy link
Member

@liujed liujed left a comment

Choose a reason for hiding this comment

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

Here's another place where authorization types are read. I think this might also need to be adjusted. https://github.com/akitasoftware/akita-libs/blob/8388976a7962059b6e7c2f1fe60d0700f62d4e4a/visitors/http_rest/spec_visitor.go#L488-L493

@mgritter
Copy link
Contributor Author

Here's another place where authorization types are read. I think this might also need to be adjusted.

https://github.com/akitasoftware/akita-libs/blob/8388976a7962059b6e7c2f1fe60d0700f62d4e4a/visitors/http_rest/spec_visitor.go#L488-L493

Do you think the context needs to be extended with the header name? Because as things stand, there's nothing more I can do in that case.

@liujed
Copy link
Member

liujed commented Jul 12, 2021

I'm not sure what we should be doing. Maybe have named be true and set name to the name of the underlying header, now that it's not just "Authorization". We'd have to check all the places where this is used to make sure the name is no longer assumed to be "Authorization".

@mgritter mgritter marked this pull request as draft March 9, 2023 17:19
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