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

fix(cloud): properly handle dev delegation #4675

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Jun 20, 2023

What this PR does / why we need it:

When a command is run with an option that results in it being run inside the dev command instead (e.g. garden deploy --sync), we now register the initial command session with Cloud as a dev command with the --cmd option set appropriately. This is useful for Cloud when listing command sessions created this way.

For good measure, we include an explicit isDevCommand parameter with the session registration request to Cloud to make this setup more robust against future changes (instead of inferring whether the command is a dev command from the commandInfo map).

Special notes for your reviewer:

I've verified that including the isDevCommand param in the registration request doesn't result in errors (it will be ignored by the API until implemented there). So there shouldn't be any compatibility problems in the meantime.

When a command is run with an option that results in it being run inside
the `dev` command instead (e.g. `garden deploy --sync`), we now register
the initial command session with Cloud as a `dev` command with the
`--cmd` option set appropriately. This is useful for Cloud when listing
command sessions created this way.

For good measure, we include an explicit `isDevCommand` parameter with
the session registration request to Cloud to make this setup more robust
against future changes (instead of inferring whether the command is a
`dev` command from the `commandInfo` map).
@thsig thsig requested a review from eysi09 June 20, 2023 20:00
core/src/cloud/api.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

@thsig thanks for fixing this! I've left a few non-blocking comments.

@vvagaytsev vvagaytsev merged commit 4cdad7f into main Jun 21, 2023
@vvagaytsev vvagaytsev deleted the fix-nested-cmd-registration branch June 21, 2023 11:45
thsig added a commit that referenced this pull request Jun 22, 2023
This fixes some problems/limitations with the changes introduced in
`4cdad7f502d88dd2b08d9bec663684b5359d87ce` (see
#4675).

Tests for the total flow will follow in a separate PR.
thsig added a commit that referenced this pull request Jun 22, 2023
This fixes some problems/limitations with the changes introduced in
`4cdad7f502d88dd2b08d9bec663684b5359d87ce` (see
#4675).

Tests for the total flow will follow in a separate PR.
eysi09 pushed a commit that referenced this pull request Jun 23, 2023
This fixes some problems/limitations with the changes introduced in
`4cdad7f502d88dd2b08d9bec663684b5359d87ce` (see
#4675).

Tests for the total flow will follow in a separate PR.
ShankyJS pushed a commit that referenced this pull request Jul 10, 2023
This fixes some problems/limitations with the changes introduced in
`4cdad7f502d88dd2b08d9bec663684b5359d87ce` (see
#4675).

Tests for the total flow will follow in a separate PR.
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.

2 participants