Skip to content

Feature | Extend Swagger Coverage for controller DistributionsApiController #359

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.

Sign up for GitHub

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

Jump to bottom
Merged
smarcet merged 15 commits into from
Nov 27, 2025
Merged

Feature | Extend Swagger Coverage for controller DistributionsApiController #359

smarcet merged 15 commits into from
Nov 27, 2025

Conversation

@matiasperrone-exo
Copy link
Contributor

@matiasperrone-exo matiasperrone-exo commented Sep 30, 2025 •
edited
Loading

Depends on:

#385

Task:

Ref: https://app.clickup.com/t/86b6wkg2j

@matiasperrone-exo matiasperrone-exo self-assigned this Sep 30, 2025
@smarcet smarcet force-pushed the main branch 2 times, most recently from 3b67b0e to 2acb44b Compare September 30, 2025 14:08
smarcet
smarcet reviewed Sep 30, 2025
View reviewed changes
smarcet
smarcet reviewed Sep 30, 2025
View reviewed changes
smarcet
smarcet requested changes Sep 30, 2025
View reviewed changes
Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@matiasperrone-exo please review comments

@smarcet smarcet force-pushed the main branch 2 times, most recently from 4f4b976 to 9ed27c0 Compare September 30, 2025 17:46
@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-openapi-documentation-to-controller-marketplace---distributionsapicontroller branch from 316faf5 to 2611055 Compare September 30, 2025 18:24
@matiasperrone-exo matiasperrone-exo marked this pull request as ready for review September 30, 2025 18:26
@matiasperrone-exo matiasperrone-exo mentioned this pull request Sep 30, 2025
Merged
@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Sep 30, 2025

@smarcet Please check that the requested changes are now complete. Thank you.

@smarcet smarcet force-pushed the main branch 2 times, most recently from e3e142a to 161d4d3 Compare October 1, 2025 02:13
smarcet
smarcet reviewed Oct 1, 2025
View reviewed changes
smarcet
smarcet requested changes Oct 1, 2025
View reviewed changes
Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@matiasperrone-exo please review

@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Oct 1, 2025

@smarcet please review, all the requested changes were incorporated.

@smarcet smarcet force-pushed the main branch 7 times, most recently from c94fc68 to 9a8387b Compare October 2, 2025 17:58
smarcet
smarcet reviewed Oct 3, 2025
View reviewed changes
smarcet
smarcet requested changes Oct 3, 2025
View reviewed changes
Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

@matiasperrone-exo please review comments

@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-openapi-documentation-to-controller-marketplace---distributionsapicontroller branch from 17c5e78 to 57cc6cf Compare October 6, 2025 19:54
smarcet
smarcet reviewed Nov 14, 2025
View reviewed changes
smarcet
smarcet reviewed Nov 14, 2025
View reviewed changes
@matiasperrone-exo matiasperrone-exo force-pushed the feature/add-openapi-documentation-to-controller-marketplace---distributionsapicontroller branch from d991309 to 84bfbca Compare November 18, 2025 19:59
@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Nov 18, 2025

@smarcet you may now the PR again, 16 new referenced schemas were added.

@matiasperrone-exo matiasperrone-exo added review Need reviewing from the developer and removed review Need reviewing from the developer labels Nov 18, 2025
@matiasperrone-exo matiasperrone-exo removed the review Need reviewing from the developer label Nov 24, 2025
@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Nov 24, 2025

@caseylocker please review

caseylocker
caseylocker requested changes Nov 24, 2025
View reviewed changes
Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

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

@matiasperrone-exo
There are at least 3 OpenAPI types that aren't valid in CompanySchema.php. Please review the OpenAPI type list. I've seen this in several PRs now. Valid types are: string, number, integer, boolean, array, object

new OA\Property(property: 'logo', type: 'url'), // Invalid type
new OA\Property(property: 'big_logo', type: 'url'), // Invalid type
new OA\Property(property: 'color', type: 'color'), // Invalid type

See app/Swagger/Models/OpenStackImplementationSchema.php for a correct implementation.

Why is there a new app/Swagger/Models/ subdirectory? Related schemas should go into the Swagger directory in related files. E.g.
app/Swagger/SummitSchemas.php - Summit-related schemas
app/Swagger/MarketplaceSchemas.php - Marketplace-related schemas
etc...

Was this change OK'd by @smarcet ?

@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Nov 24, 2025 •
edited
Loading

@matiasperrone-exo There are at least 3 OpenAPI types that aren't valid in CompanySchema.php. Please review the OpenAPI type list. I've seen this in several PRs now. Valid types are: string, number, integer, boolean, array, object

new OA\Property(property: 'logo', type: 'url'), // Invalid type new OA\Property(property: 'big_logo', type: 'url'), // Invalid type new OA\Property(property: 'color', type: 'color'), // Invalid type

See app/Swagger/Models/OpenStackImplementationSchema.php for a correct implementation.

Why is there a new app/Swagger/Models/ subdirectory? Related schemas should go into the Swagger directory in related files. E.g. app/Swagger/SummitSchemas.php - Summit-related schemas app/Swagger/MarketplaceSchemas.php - Marketplace-related schemas etc...

Was this change OK'd by @smarcet ?

@caseylocker thanks for the comments.
Regarding the app/Swagger/Models/ subdirectory, we created it to avoid collisions when working on parallel with @andrestejerina97 on shared Model's schemas that may be the same in some controller's response schemas, and also as a workaround for any conflict that may arise. There was no exact definition and because they are used in multiple places seemed like a good place to start. we can merge these in the future.

@matiasperrone-exo
Copy link
Contributor Author

matiasperrone-exo commented Nov 25, 2025

@smarcet this new directory is an attempt to be easily maintainable and to fix and detect conflicts in the different branches to be ready to be merged to main at the end with no problems

caseylocker
caseylocker approved these changes Nov 25, 2025
View reviewed changes
Copy link

@caseylocker caseylocker left a comment

Choose a reason for hiding this comment

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

My comments have been addressed. @smarcet if you agree please merge.

smarcet
smarcet approved these changes Nov 27, 2025
View reviewed changes
Copy link
Collaborator

@smarcet smarcet left a comment

Choose a reason for hiding this comment

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

LGTM

@smarcet smarcet merged commit f74501e into main Nov 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smarcet smarcet smarcet approved these changes

@caseylocker caseylocker Awaiting requested review from caseylocker

Assignees

@matiasperrone-exo matiasperrone-exo

Labels

documentation Improvements or additions to documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matiasperrone-exo @caseylocker @smarcet @matiasperrone