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: make get_domains parameters optional #2278

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

Conversation

tien
Copy link

@tien tien commented Feb 25, 2025

I'm trying to generate a client from this OpenAPI spec. And it appears that the parameter type for get_domains is incorrect.

@yubiuser
Copy link
Member

Please base your PR on and target to development branch.

@tien tien changed the base branch from master to development February 26, 2025 02:24
DL6ER
DL6ER previously approved these changes Feb 26, 2025
@tien
Copy link
Author

tien commented Feb 27, 2025

Looks like the validator is just buggy and won't accept the allOf syntax :v

So I've replaced it with another one, also with added bonus of this one working with relative paths.

Signed-off-by: Tiến Nguyễn Khắc <[email protected]>
@DL6ER
Copy link
Member

DL6ER commented Feb 27, 2025

This is too much of a change, looking at various documentation, e.g. Swagger it seems what you want to add is not valid/possible plus invalid syntax.

Instead of

        parameters:
          - allOf:
            - $ref: 'domains.yaml#/components/parameters/type'
            - required: false
          - allOf:
            - $ref: 'domains.yaml#/components/parameters/kind'
            - required: false
          - allOf:
            - $ref: 'domains.yaml#/components/parameters/domain'
            - required: false

it should rather be

        parameters:
            - $ref: 'domains.yaml#/components/parameters/type'
              required: false
            - $ref: 'domains.yaml#/components/parameters/kind'
              required: false
            - $ref: 'domains.yaml#/components/parameters/domain'
              required: false

but even this is invalid because of
grafik
(screenshot from my link above) and would need to be added further up in this endpoint definition (there is already a parameters object).

The validator change you suggest does not solve the issue, it just doesn't check for this requirement of the OpenAPI specs and, hence, is incomplete.


What we'd need instead is to define new endpoints and use referencing to keep copy-pasting minimal:

+GET /domains
+GET /domains/{type}
+GET /domains/{type}/{kind}
 GET /domains/{type}/{kind}/{domain}

All other (POST, DELETE, PUT) require the parameters. And this then also for

+GET /groups
 GET /groups/{name}
+GET /clients
 GET /clients/{client}
+GET /lists
 GET /lists/{list}

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