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

Resove issue #1623-Devops API swagger #1628

Merged
merged 8 commits into from
May 10, 2023

Conversation

neottil
Copy link
Contributor

@neottil neottil commented Apr 28, 2023

I create new devops-index.yml file for creation of ditto-devops.yml swagger file. The new files referred as path, parameter, schemas have been divided following the existing example. I reported what I found in the documentation and by looking at the code.
Please take a look and suggest correction if needed.
This new swagger file will have to be integrated into the helm project, could you give me some information about it? The api swagger i saw is made available via a url.

i have also fixed an openapi error: "Semantic error at paths./things/{thingId}/features.patch.parameters.1.name
Path parameter "featureId" must have the corresponding {featureId} segment in the "/things/{thingId}/features" path"
Luca

Resolves: #1623

neottil added 3 commits April 20, 2023 17:59
Semantic error at paths./things/{thingId}/features.patch.parameters.1.name
Path parameter "featureId" must have the corresponding {featureId} segment in the "/things/{thingId}/features" path
Jump to line 2071
@thjaeckle
Copy link
Member

Hi @neottil
That looks like a lot of effort you put into that PR - thanks a lot.
I will soon take a look.

What I noticed: Please adjust the year in the copyright header for each new file to 2023 - the file creation year must be in the copyright header.

Did you have a specific reason to provide another top-level OpenAPI file and not merge the "devops" commands in the main OpenAPI file?
Because we also added the "/connections" API in there - which already requires "DevOps" authentication as well..

@neottil
Copy link
Contributor Author

neottil commented Apr 28, 2023

Hi,
Sorry for the header, I didn't pay attention and copied it from an existing file.
I preferred to separate the files because for my needs I developed it in this way and I thought they could be annoying together with the other APIs. I usually tend to divide as much as possible also to be easier to maintain. If you prefer I can move into one file.

@thjaeckle
Copy link
Member

If you prefer I can move into one file.

I would really prefer that - otherwise we would also need to provide different APIs to the "dropdown".
And that way you have all existing HTTP APIs of Ditto at a glance 👍

@neottil
Copy link
Contributor Author

neottil commented Apr 28, 2023

Ok, so I add the servers and the tags to the already existing file? The authentication will have to be done as for /connections

@thjaeckle
Copy link
Member

Ok, so I add the servers and the tags to the already existing file? The authentication will have to be done as for /connections

Yes, all to the ditto-api-2.yml

Cool that you figured out so quickly how the build and module mechanism works there 💯

@neottil
Copy link
Contributor Author

neottil commented Apr 28, 2023

Will it be clear that the new API's will need to be run by selecting the /devops server?
Is it worth adding a note in the description of the swagger?

@thjaeckle
Copy link
Member

True, I did not think about that caused by the different entry point in the API.

Can the devops resources then only be shown if the /devops server is chosen?

@neottil
Copy link
Contributor Author

neottil commented Apr 28, 2023 via email

@neottil
Copy link
Contributor Author

neottil commented Apr 29, 2023

I've been looking for a solution to hide the APIs based on server selection, but I haven't found anything. I discovered the possibility of using variables also in the server path, in this way we could configure only 2 servers (one sandbox and one local) by parameterizing the final part (api/2 or devops). https://swagger.io/docs/specification/api-host-and-base-path/ (Server Templating paragraph)
However, this does not solve the problem. Being 2 different endpoints we should document which APIs belong to one and which to the other.
Maybe splitting into two files would be better, but I don't know what other actions that entails and their efford.

@thjaeckle
Copy link
Member

Or, we could add the /api/2 prefix to a existing endpoints in the openapi docs.
That way our entry point would be /.

Could however be a little overwhelming, I did not yet try it out and reviewed in swagger ui.

@neottil
Copy link
Contributor Author

neottil commented Apr 30, 2023

I await your news and instructions

@thjaeckle
Copy link
Member

Please generate all apis to a single file, change the servers to serve at / and add the api/2 to all existing apis..
Splitting in 2 api definition files with 2 servers each would be really confusing.

neottil and others added 2 commits May 1, 2023 14:37
… old api entry point in /api/2. Add new devops api with entry point /devops and devops credentials.
@neottil
Copy link
Contributor Author

neottil commented May 1, 2023

I've modify swagger file such agreed. If there are any other details to review let me know.
I hope my work can help.

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

Hi @neottil

Looks already very promising, thanks a lot.
Could you please add "DevOps: " as prefix for the new sections.

That way those are "grouped" a little bit better.
image

Or should/could we even just have 1 category instead of 3 for "DevOps"?
What do you think?

Edit: I think it would be sufficient to have a single "DevOps Resources" section combining the devops endpoints.

@neottil
Copy link
Contributor Author

neottil commented May 3, 2023

I think that as api/2 is split into groups devops should be too. As already mentioned, I think it's always better to divide where you can. It can help in code management. However, if you prefer to have only one devops group because it is more understandable, I will do this.

@thjaeckle
Copy link
Member

I think it is too overwhelming to have that much groups on top level for the "operator" (Admin) of the Ditto installation.
The APIs for the developers using the Ditto API should be the focus.

I would therefore really prefer to only have a single "Devops" section.

@thjaeckle
Copy link
Member

@neottil I will have a more detailed look soon.
Thanks a lot for your contribution.

Regarding availability in the Helm chart: the Helm chart downloads the openapi definition file for its app version via GitHub.
So once the next Ditto and helm chart Release is done, the new Openapi file will automatically be included.

Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

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

@neottil I finally got the chance to have a close look and try it out.
Great contribution - thanks a lot 👍
LGTM

@thjaeckle thjaeckle merged commit 6f7a336 into eclipse-ditto:master May 10, 2023
@thjaeckle thjaeckle added this to the 3.3.0 milestone May 10, 2023
@neottil neottil deleted the neottil/#1621 branch August 20, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Devops API swagger
2 participants