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

[FLOW-10732] Secrets API #8

Merged
merged 10 commits into from
Feb 18, 2025
Merged

[FLOW-10732] Secrets API #8

merged 10 commits into from
Feb 18, 2025

Conversation

jessmand
Copy link
Contributor

@jessmand jessmand commented Jan 31, 2025

@jessmand
Copy link
Contributor Author

@sayanb06 tagged you for review to make sure everything is correct since you wrote the API!

Copy link

@sayanb06 sayanb06 left a comment

Choose a reason for hiding this comment

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

Couple nits, lgtm otherwise!

openapi.yaml Outdated
- NONE
allowed_workspaces:
type: array
description: Which workspaces are allowed if only SOME workspaces are allowed.

Choose a reason for hiding this comment

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

Can we edit this to be something like:
"If allowed_workspaces_type is SOME, then specify which workspaces in particular. The language is a bit off but it makes it clear that allowed_workspaces_type == SOME

Choose a reason for hiding this comment

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

It seems this is also specified in the example so feel free to ignore if not necessary.

openapi.yaml Outdated
alias:
type: string
description: Name of the secret.
description:

Choose a reason for hiding this comment

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

optional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a list of required props below, I assume this gets integrated into the description in an automated way

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, any parameters not listed as “required” in the special “required” section are listed as “optional” in the parameter descriptions.

openapi.yaml Outdated
-H "Authorization: Bearer ${API_TOKEN}" \
-H "IB-Context: ${IB_CONTEXT}"\
-F alias="my_secret" \
-F description="Sample secret" \

Choose a reason for hiding this comment

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

optional comment

openapi.yaml Outdated

result = client.secrets.create(
alias="my_secret",
description="Sample secret",

Choose a reason for hiding this comment

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

same as above

openapi.yaml Outdated
Comment on lines 2635 to 2654
description:
type: string
description: New description of the secret.
value:
type: string
description: New content of the secret.
allowed_workspaces_type:
type: string
description: New designation of which workspaces in this organization are allowed to use this secret in custom functions.
enum:
- ALL
- SOME
- NONE
allowed_workspaces:
type: array
description: New designation of which workspaces are allowed if only SOME workspaces are allowed.
items:
type: string
required:
- alias

Choose a reason for hiding this comment

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

We should say that this is as needed. If not specified, then the value will stay the same.

openapi.yaml Outdated
description: The user ID of the user who last edited the secret.

/v2/secrets:
post:

Choose a reason for hiding this comment

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

Do we need to include the GET API? The one above just seems to indicate what the object type returned is. In addition, same question for getting deployed solutions affected by secret.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh crap I must have accidentally deleted 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.

I was intending on not adding the affected deployments endpoint though

Copy link

@sayanb06 sayanb06 left a comment

Choose a reason for hiding this comment

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

nice! will let others stamp but from an API Perspective, this looks correct

openapi.yaml Outdated
description: |
List all secrets available to the organizaiton, in a given workspace, and with a given alias

<Note>Only organization admins can list all secrets in the organization.</Note>

Choose a reason for hiding this comment

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

Can we also add that nonadmins must specify workspace?

Copy link
Collaborator

@cwcowell cwcowell left a comment

Choose a reason for hiding this comment

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

Wow, thanks for all the great work on this! Super clear.

openapi.yaml Outdated
- secrets
summary: List secrets
description: |
List all secrets available to the organizaiton, in a given workspace, and with a given alias
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
List all secrets available to the organizaiton, in a given workspace, and with a given alias
List secrets available to the organization, secrets in a given workspace, or secrets with a given alias.

Copy link
Collaborator

@cwcowell cwcowell Jan 31, 2025

Choose a reason for hiding this comment

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

Can you have the same alias defined in two different workspaces, corresponding to different secrets?

If not, maybe use this instead? List secrets available to the organization, secrets available in a given workspace, or the secret with a given alias.

openapi.yaml Outdated
description: |
List all secrets available to the organizaiton, in a given workspace, and with a given alias

<Note>Only organization admins can list all secrets in the organization.</Note>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Note>Only organization admins can list all secrets in the organization.</Note>
<Note>Only organization admins can list all secrets in the organization. Non-admins must specify a workspace.</Note>

openapi.yaml Outdated
schema:
type: string
description: |
Filter to secrets in the specified workspace.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Filter to secrets in the specified workspace.
Show secrets defined in the specified workspace.

openapi.yaml Outdated
description: |
Filter to secrets in the specified workspace.

<Info>In order to get the secrets for a given workspace, the user must be a member of that workspace.</Info>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Info>In order to get the secrets for a given workspace, the user must be a member of that workspace.</Info>
<Info>Only members of a workspace can get secrets defined in that workspace.</Info>

openapi.yaml Outdated
schema:
type: string
description: |
Get only the secret with the specified alias.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Get only the secret with the specified alias.
Get the secret with the specified alias.

openapi.yaml Outdated
properties:
alias:
type: string
description: Name of the secret to be edited.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: Name of the secret to be edited.
description: Name of the secret to update.

openapi.yaml Outdated
- secrets
summary: Update secret
description: |
Update the value, description, or allowed workspaces for a secret.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Update the value, description, or allowed workspaces for a secret.
Update the description, value, or allowed workspaces for a secret.

To preserve the same order as the parameters are listed below.

openapi.yaml Outdated
- NONE
allowed_workspaces:
type: array
description: New designation of which workspaces are allowed if only SOME workspaces are allowed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: New designation of which workspaces are allowed if only SOME workspaces are allowed.
description: New designation of which workspaces are allowed to use this secret, if only some are.

openapi.yaml Outdated
properties:
alias:
type: string
description: Name of the secret to be deleted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: Name of the secret to be deleted.
description: Name of the secret to delete.

openapi.yaml Outdated
alias:
type: string
description: Name of the secret.
description:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, any parameters not listed as “required” in the special “required” section are listed as “optional” in the parameter descriptions.

@jessmand jessmand requested a review from cwcowell February 3, 2025 17:27
- secrets
summary: Update secret
description: |
Update the description, value, or allowed workspaces for a secret. If a value for a parameter is not specified, it will remain the same.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwcowell what do you think of this second sentence I added here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 helpful clarification

Copy link
Collaborator

@anilshanbhag anilshanbhag left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Do you have a PR for addition to SDK/tests already?

@anilshanbhag
Copy link
Collaborator

We should merge both the main repo PR and this at the same time.

@jessmand
Copy link
Contributor Author

jessmand commented Feb 3, 2025

@anilshanbhag I do not yet, trying to get that done today - I might have to backport it. I'll wait to merge this until that's done.

@cwcowell
Copy link
Collaborator

Checking status on this, so we can merge https://github.com/instabase/instabase-fern/pull/181 at the same time and make sure we put release notes for this change under the correct AI Hub version.

@jessmand are we waiting on the corresponding SDK & tests PR, or is this ready to merge?

Copy link
Collaborator

@cwcowell cwcowell left a comment

Choose a reason for hiding this comment

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

LGTM

openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
openapi.yaml Outdated Show resolved Hide resolved
- code-samples:
- sdk: curl
code: |
curl -X PATCH "${API_ROOT}/v2/secrets" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow I’ve never seen PATCH used as an HTTP verb literally ever in my career. 🎉

Co-authored-by: Chris Cowell <43705622+cwcowell@users.noreply.github.com>
@jessmand jessmand merged commit 05c2fb4 into master Feb 18, 2025
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.

5 participants