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.

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

Fix swagger issue in the instance rebuild endpoint #12137

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Fix swagger issue in the instance rebuild endpoint #12137

merged 2 commits into from
Aug 16, 2023

Conversation

gabrielmougard
Copy link
Contributor

No description provided.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Aug 10, 2023
ru-fu
ru-fu reviewed Aug 10, 2023
View reviewed changes
Copy link
Contributor

@ru-fu ru-fu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Have you tested this?
I don't know enough about the syntax to be sure, but the current output suggests that there is something wrong with the parameter syntax (since they don't show up correctly).

doc/rest-api.yaml Outdated
@@ -8311,7 +8311,7 @@ paths:
summary: Bulk instance state update
tags:
- instances
/1.0/instances/{instance_name}/rebuild:
/1.0/instances/{instance}/rebuild:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/1.0/instances/{instance}/rebuild:
/1.0/instances/{name}/rebuild:

doc/rest-api.yaml Outdated
@@ -8311,7 +8311,7 @@ paths:
summary: Bulk instance state update
tags:
- instances
/1.0/instances/{instance_name}/rebuild:
/1.0/instances/{instance}/rebuild:
post:
consumes:
- application/octet-stream
Copy link
Contributor

Choose a reason for hiding this comment

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

The operation ID (just below this line) is duplicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other sections all have a description field as well.

@gabrielmougard gabrielmougard force-pushed the fix/rebuild-api-comments branch 2 times, most recently from fe59d30 to 74dd387 Compare August 11, 2023 11:08
@ru-fu
Copy link
Contributor

ru-fu commented Aug 11, 2023

This looks better, except that there still is an empty object field: https://canonical-ubuntu-documentation-library--12137.com.readthedocs.build/lxd/en/12137/api/#/instances/instance_rebuild_post

@gabrielmougard gabrielmougard force-pushed the fix/rebuild-api-comments branch 2 times, most recently from 2401029 to 3d1ea0d Compare August 14, 2023 14:52
@tomponline
Copy link
Member

Is this ready to be merged?

tomponline
tomponline reviewed Aug 15, 2023
View reviewed changes
shared/api/instance.go Outdated
@@ -174,7 +174,7 @@ type InstancePut struct {
// API extension: instances_rebuild.
type InstanceRebuildPost struct {
// Rebuild source
Source InstanceSource `json:"source" yaml:"source"`
Source InstanceSource `yaml:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

This is an api breakage I don't think we can change it.

What was the thinking behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just a test to see the output of the readthedoc documentation but it does not seem to be what I want. Now I reverted it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this https://canonical-ubuntu-documentation-library--12137.com.readthedocs.build/lxd/en/12137/api/#/instances/instance_rebuild_post looks good so I'd say this is ready to be merged. What do you think @ru-fu ?

ru-fu
ru-fu reviewed Aug 16, 2023
View reviewed changes
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Let's stay consistent with how the other entries do it - a short summary and a longer description.
Otherwise this looks good to me.

doc/rest-api.yaml
post:
consumes:
- application/octet-stream
operationId: instance_rebuild_post
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
operationId: instance_rebuild_post
description: Rebuild an instance using an alternate image or as empty.
operationId: instance_rebuild_post

doc/rest-api.yaml Outdated
$ref: '#/responses/NotFound'
"500":
$ref: '#/responses/InternalServerError'
summary: Rebuild an instance using an alternate image or as empty.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
summary: Rebuild an instance using an alternate image or as empty.
summary: Rebuild an instance

@gabrielmougard
lxd: update swagger description in rebuild endpoint
689e6c4 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
@gabrielmougard
doc: update rest-api.yaml
5adb102 
Signed-off-by: Gabriel Mougard <gabriel.mougard@canonical.com>
ru-fu
ru-fu approved these changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

LGTM

@tomponline tomponline merged commit d423731 into canonical:main Aug 16, 2023
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline left review comments

@ru-fu ru-fu ru-fu approved these changes

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabrielmougard @ru-fu @tomponline