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

feat: Remove name from configuration response #22

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

Conversation

grimly
Copy link
Contributor

@grimly grimly commented May 30, 2024

This PR

  • Removes name from the GET /ofrep/v1/configuration response

Notes

This property provides no added value to the API client.

While I understand why a vendor want to promote his product by name, this is the role of the Server HTTP header.

@grimly grimly requested a review from a team as a code owner May 30, 2024 20:46
@grimly grimly changed the title Remove name from configuration response feat: Remove name from configuration response May 30, 2024
@thomaspoignant
Copy link
Member

The goal of the name is to be able to use it in the providers for logging and observability purposes (such as building otel traces etc ...).

If you use multiple OFREP providers in your application having the name would be important to be able to differentiate where the things happen.

I am not sure to completely understand why you think it should be removed?

@grimly
Copy link
Contributor Author

grimly commented Jun 1, 2024

You can put this information on the Server header.
It can even be sent on all evaluations, therefore providing this information even on those traces

@Kavindu-Dodan
Copy link
Contributor

I think name can be an optional property. While I agree this could be resolved with headers, that adds some extra complexity at the OFREP implemetaiton.

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.

3 participants