-
Notifications
You must be signed in to change notification settings - Fork 175
Update sandbox metadata #956
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
base: main
Are you sure you want to change the base?
Conversation
e88ce2b
to
506a4bf
Compare
506a4bf
to
7d3b28a
Compare
the description looks to be incorrect |
My main question is about the API—which of the following do we want:
Also if users wants to merge/only add metadata that should be somehow possible. |
5df89a9
to
bdb97cd
Compare
bdb97cd
to
c651166
Compare
tests/integration/internal/tests/api/sandbox_metadata_update_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/api/sandbox_metadata_update_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/api/sandbox_metadata_update_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/api/sandbox_metadata_update_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/api/sandbox_metadata_update_test.go
Outdated
Show resolved
Hide resolved
@cursoragent review |
This comment was marked as resolved.
This comment was marked as resolved.
func (o *Orchestrator) UpdateSandboxMetadata( | ||
ctx context.Context, | ||
sbx *instance.InstanceInfo, | ||
metadata map[string]string, | ||
) *api.APIError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a missing lock for concurrent metadata updates? e.g. first request succeeds second on the orchestrator, but second request sets the metadata on the API -> inconsistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactor how we lock, now if there's some handler or background process working with sandbox it should always acquire lock
assert.Equal(t, count, successCount, "All concurrent updates should succeed") | ||
|
||
// Verify final state - should be from one of the updates | ||
getSandboxResponse, err := c.GetSandboxesSandboxIDWithResponse(t.Context(), sandbox.SandboxID, setup.WithAPIKey()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't check that the metadata reported by the API is the same as the one reported by the orchestrator
Description
This pull request introduces support for setting sandbox metadata via a new PUT endpoint in the API. These changes allow clients to set the metadata of a running sandbox.