Skip to content

[ENG-8143] Add GravyValet page #79

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

opaduchak
Copy link

@opaduchak opaduchak commented Jul 9, 2025

Purpose

Add API documentation for GV

Ticket

https://openscience.atlassian.net/browse/ENG-8143

@opaduchak opaduchak changed the title Add GrtavyValet page Add GravyValet page Jul 9, 2025
@opaduchak opaduchak changed the title Add GravyValet page [ENG-8143] Add GravyValet page Jul 9, 2025
Copy link
Collaborator

@cslzchen cslzchen left a comment

Choose a reason for hiding this comment

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

Looks good but is it possible to have a screenshot of how it will look?

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

Mostly some super minor typos and such, from what I could understand of these docs. Not a blocker for merge IMO. Awesome work here!

- addon-operation-invocations
/v1/addon-operation-invocations/{id}/:
get:
description: Get singular instance of addon operation invocation by it's pk.
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
description: Get singular instance of addon operation invocation by it's pk.
description: Get singular instance of addon operation invocation by its pk.

Comment on lines +6752 to +6867
- account_owner
- api_base_url
- auth_url
- authorized_capabilities
- authorized_operations
- authorized_operation_names
- configured_storage_addons
- credentials
- default_root_folder
- external_storage_service
- initiate_oauth
- credentials_available
type: string
type: array
- description: A UUID string identifying this Authorized Storage Account.
in: path
name: id
required: true
schema:
format: uuid
type: string
- description: include query parameter to allow the client to customize which
related resources should be returned.
explode: false
in: query
name: include
schema:
items:
enum:
- account_owner
- external_storage_service
- configured_storage_addons
- authorized_operations
type: string
type: array
responses:
'200':
content:
application/vnd.api+json:
schema:
$ref: '#/components/schemas/AuthorizedStorageAccountResponse'
description: ''
tags: *id001
/v1/addon-operation-invocations/{id}/thru_addon:
get:
description: Fetch AddonOperationInvocation's ConfiguredStorageAddon
operationId: addon_operation_invocations_retrieve_2_related_thru_addon
parameters:
- description: endpoint return only specific fields in the response on a per-type
basis by including a fields[TYPE] query parameter.
explode: false
in: query
name: fields[configured-storage-addons]
schema:
items:
enum:
- id
- url
- display_name
- root_folder
- base_account
- authorized_resource
- authorized_resource_uri
- connected_capabilities
- connected_operations
- connected_operation_names
- external_storage_service
- current_user_is_owner
- external_service_name
type: string
type: array
- description: A UUID string identifying this Configured Storage Addon.
in: path
name: id
required: true
schema:
format: uuid
type: string
- description: include query parameter to allow the client to customize which
related resources should be returned.
explode: false
in: query
name: include
schema:
items:
enum:
- base_account
- external_storage_service
- authorized_resource
- connected_operations
type: string
type: array
responses:
'200':
content:
application/vnd.api+json:
schema:
$ref: '#/components/schemas/ConfiguredStorageAddonResponse'
description: ''
tags: *id001
Copy link
Contributor

Choose a reason for hiding this comment

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

These two section seem to imply that the thru_account and thru_addon relationships are only AuthorizedStorageAccount and ConfiguredStorageAddons. Is it the case that these could eventually be any kind of AuthorizedAccount or ConfiguredAddon?

type: string
type: array
responses:
'200'
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
'200'
description: Get resource reference by its pk

type: string
type: array
responses:
'200'
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar typo and question here re: list endpoint returning only one thing

Suggested change
'200'
description: Get user reference by user_uri. Even though this is a list method,

type: string
type: array
responses:
'200'
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
'200'
description: Get user reference by its pk

@futa-ikeda
Copy link
Contributor

Huh, some of my review comments are not pointing at the correct lines... let me see if I can fix that

Comment on lines 7446 to 8681
- authorized_operations
type: string
type: array
responses:
'200'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I've set up a computing account but is this true that you need a root_folder for setting up a configured-computing-addon?

type: string
type: array
responses:
'200'
Copy link
Contributor

Choose a reason for hiding this comment

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

One typo and one comment. Not really within the scope of this PR, but this feels a bit strange to me that this is a list view, but we only expect to show one item. Maybe it's not too weird though 🤷

Suggested change
'200'
description: Get resource reference by resource_uri. Even though this is a

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.

3 participants