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

Add API-specification #28

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Add API-specification #28

merged 1 commit into from
Jan 19, 2022

Conversation

vetlek
Copy link
Contributor

@vetlek vetlek commented Jan 13, 2022

No description provided.

@vetlek vetlek force-pushed the api_contract branch 7 times, most recently from 8d7fc8e to 119f08c Compare January 14, 2022 08:45
@github-pages github-pages bot temporarily deployed to github-pages January 14, 2022 08:50 Inactive
@vetlek vetlek marked this pull request as ready for review January 14, 2022 08:54
@github-pages github-pages bot temporarily deployed to github-pages January 14, 2022 08:57 Inactive
Copy link

@nicholasdalhaug nicholasdalhaug left a comment

Choose a reason for hiding this comment

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

👍

docs/openapi.yaml Outdated Show resolved Hide resolved
@vetlek vetlek changed the title Add api contract Add API-specification Jan 14, 2022
docs/openapi.yaml Outdated Show resolved Hide resolved
InspectionType:
type: string
description: Valid inspection types
enum: ["thermal_image", "image", "audio"]
Copy link
Member

Choose a reason for hiding this comment

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

Should InspectionType, ReportStatus, and RobotStatus enums be reviewed and aligned with the enums in the database pr #32?

summary: Lookup map of the area where the robot is located
description: |
### Overview
Lookup map of the area where the robot is located
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return the map or the name of the map?

### Overview
Adds a new event to the robot schedule

New entries to the schedule can be added as long as they don't conflict with already scheduled events.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would a conflict look like? Is it if they are scheduled for the same time? Should this by default add to mission queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conflict is that the new event overlaps in time with already scheduled events. I think the user then should be informed about the conflict and which event(s) it is in conflict with instead of some default action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, that this should be the behaviour once some time for the inspection has been requested by the user, but should the default behaviour be that it is just entered into a queue and the user would then specify a specific time window if that is desired? I would guess that for many missions one does not care that much about when it is performed, just that it happens within a reasonable time

docs/openapi.yaml Outdated Show resolved Hide resolved
summary: Start a mission with robot
description: |
### Overview
Start a mission with given id using robot with robot id.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we start it immediatly and/or at a given time?

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 think it should start immediately and create event in the case we want to start it at a given time.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have some queue system for each robot instance, would this then add it to that queue so it starts at its earliest convenience or do we require that the robot is ready to start a new mission prior to using this endpoint?

summary: Lookup all available missions on asset
description: |
### Overview
Lookup up all available missions on asset in the Echo mission planner
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we also list the draft missions in some manner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't thought about it. Depends on the overall workflow we want. I guess it would be natural that draft missions isn't shown outside of the echo mission planner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this would probably become clear after the process with UX designer is competed

@nicholasdalhaug
Copy link

Are there a bit too many endpoints? I understand we might want more functionality over time, but maybe we should start on a simpler level. Maybe just a few endpoints?

docs/openapi.yaml Show resolved Hide resolved
docs/openapi.yaml Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
docs/openapi.yaml Outdated Show resolved Hide resolved
@github-pages github-pages bot temporarily deployed to github-pages January 18, 2022 10:24 Inactive
@github-pages github-pages bot temporarily deployed to github-pages January 18, 2022 12:40 Inactive
Copy link

@nicholasdalhaug nicholasdalhaug left a comment

Choose a reason for hiding this comment

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

I still think there are too many endpoints to start with

docs/index.html Outdated Show resolved Hide resolved
@github-pages github-pages bot temporarily deployed to github-pages January 19, 2022 08:31 Inactive
Copy link
Contributor

@Christdej Christdej left a comment

Choose a reason for hiding this comment

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

LGTM, this will change many times during implementation anyways, so I say we go for it and change it when we see fit

@vetlek vetlek merged commit 3553d0a into equinor:main Jan 19, 2022
@vetlek vetlek deleted the api_contract branch January 19, 2022 10:47
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.

7 participants