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: reversed lack of support to support capabilities #19

Closed
wants to merge 1 commit into from

Conversation

grimly
Copy link
Contributor

@grimly grimly commented May 28, 2024

This PR

  1. Reverses the unsupportedTypes to supportedTypes
  2. Removes enumeration of supported types
  3. Restores the required name in the configurationResponse

Related Issues

N/A

Notes

  1. It is not a capability to not support something.
    Instead the response should express its ability to support such types.
    I reversed the naming of this capability because as this specification updates, this would force providers either to immediately show a new type to be unsupported or to immediately support a new type.
  2. I removed the limitations of supported types. Adding values to an enum in a response is a breaking change. The goal is to allow a new type in the OpenFeature specification while limiting breaking changes on this OpenAPI specification.
  3. I think this required clause was wrongly moved. I'm merely bringing it back up.
    I would nevertheless question the requirement of this property as it does not add any significant value to a client.

Follow-up Tasks

N/A

How to test

N/A

@grimly grimly requested a review from a team as a code owner May 28, 2024 20:16
@thomaspoignant
Copy link
Member

thomaspoignant commented May 29, 2024

We had a discussion with the community about this and went for unsupportedTypes on purpose to be sure that the default value is to accept everything.

We are missing an ADR about this, I will open an issue to add one about this.
We will probably not reverse this logic.

Pinging @Kavindu-Dodan @lukas-reining @beeme1mr to have confirmation.

@thomaspoignant
Copy link
Member

Issue to add the ADR is available here: #21

@Kavindu-Dodan
Copy link
Contributor

This overlaps with #24

@Kavindu-Dodan
Copy link
Contributor

@grimly #24 reverted the unsupported types to supported types. I will close this PR as a cleanup.

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