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

Swagger updates for global and output configuration #103

Merged

Conversation

bhufmann
Copy link
Contributor

@bhufmann bhufmann commented Oct 29, 2024

  • Update documentation of Configuration and ConfigurationQueryParameters.
    Add name and description ConfigurationQueryParameters used for global configuration.
  • Add configuration service per output (data provider)
    This addition enables clients to create derived data providers from an existing data provider. This change also includes updates the "DataProvider" descriptor to include optional parent ID and configuration object to be included in derived data providers.

Signed-off-by: Bernd Hufmann bernd.hufmann@ericsson.com

@@ -225,7 +225,7 @@ public Response putConfiguration(@Parameter(description = CFG_TYPE_ID) @PathPara
@RequestBody(description = CFG_UPDATE_DESC + " " + CFG_KEYS_DESC, content = {
@Content(examples = @ExampleObject("{\"parameters\":{" + CFG_PATH_EX +
"}}"), schema = @Schema(implementation = org.eclipse.tracecompass.incubator.internal.trace.server.jersey.rest.core.model.ConfigurationQueryParameters.class))
}, required = true) ConfigurationQueryParameters queryParameters) {
}, required = true) InputConfigurationQueryParameters queryParameters) {
Copy link
Contributor

@PatrickTasse PatrickTasse Oct 29, 2024

Choose a reason for hiding this comment

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

Maybe there is an issue with Swagger generation that would prevent this that I don't understand, but would it be possible to have the same concept in the views package with views.ConfigurationQueryParameters here without typeId and views.OutputConfigurationQueryParameters that extends it to add typeId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it as you suggested, plus changed the swagger doc generation to use allOf schema tag for inheritance. It seems to work and it's cleaner.

The documentation didn't match the implementation. This PR fixes this.

Signed-off-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
Copy link
Contributor

@PatrickTasse PatrickTasse left a comment

Choose a reason for hiding this comment

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

The example object in ConfigurationManagerService (POST and PUT) is missing 'name' which is mandatory, and perhaps 'description' which is optional.
I noticed that in DataProviderService we don't provide any example object and the example is automatically generated with all parameters with default values. Perhaps this one here was made explicit to provide "path" as an example?

@bhufmann
Copy link
Contributor Author

bhufmann commented Oct 30, 2024

The example object in ConfigurationManagerService (POST and PUT) is missing 'name' which is mandatory, and perhaps 'description' which is optional. I noticed that in DataProviderService we don't provide any example object and the example is automatically generated with all parameters with default values. Perhaps this one here was made explicit to provide "path" as an example?

I can update the example.

For the dp configuration, I didn't put an explicit example, because an example is generated. Generated examples have less details, though.

Signed-off-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
@bhufmann bhufmann merged commit d48d88d into eclipse-tracecompass-incubator:master Oct 30, 2024
2 checks passed
@bhufmann bhufmann deleted the swagger-updates branch October 30, 2024 15:24
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