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(dev): fix unresolved templates in cmd results #6850

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Conversation

thsig
Copy link
Collaborator

@thsig thsig commented Feb 13, 2025

What this PR does / why we need it:

We were including the raw graph results in the results of commands requested over HTTP from the live page. This was always unnecessary (and the data structures involved were excessively large), but only started resulting in errors with our recent templating improvements (previously, we'd just leave the unresolved template strings in there).

Which issue(s) this PR fixes:

Fixes live-mode commands run from the Cloud UI to fail under certain conditions.

eysi09
eysi09 previously approved these changes Feb 13, 2025
@thsig
Copy link
Collaborator Author

thsig commented Feb 14, 2025

There are some test failures here. I'll take a look.

@thsig thsig force-pushed the dev-command-fixes branch 5 times, most recently from 8109854 to 986db2e Compare February 16, 2025 22:35
We were including the raw graph results in the results of commands
requested over HTTP from the live page. This was always unnecessary (and
the data structures involved were excessively large), but only started
resulting in errors with our recent templating improvements (previously,
we'd just leave the unresolved template strings in there).

We can clean this up further in 0.14 when we get rid of `graphResults`
from `ProcessCommandResult` (which will require fixing a bunch of test
cases which currently use that field).
@thsig thsig force-pushed the dev-command-fixes branch from 986db2e to f170f83 Compare February 16, 2025 22:45
@eysi09 eysi09 added this pull request to the merge queue Feb 17, 2025
Merged via the queue into main with commit 4a2cd9c Feb 17, 2025
40 checks passed
@eysi09 eysi09 deleted the dev-command-fixes branch February 17, 2025 08:13
thsig added a commit that referenced this pull request Feb 20, 2025
This should fix a regression introduced in
#6850.
thsig added a commit that referenced this pull request Feb 20, 2025
This should fix a regression introduced in
#6850.

Because we added a lookup into `graphResult.result` and
`graphResult.result` can be null in certain circumstances (e.g. when the
action fails), we needed to guard against this (since we don't want an
internal error for such cases).
thsig added a commit that referenced this pull request Feb 20, 2025
This should fix a regression introduced in
#6850.

Because we added a lookup into `graphResult.result` and
`graphResult.result` can be null in certain circumstances (e.g. when the
action fails), we needed to guard against this (since we don't want an
internal error for such cases).
thsig added a commit that referenced this pull request Feb 20, 2025
This should fix a regression introduced in
#6850.

Because we added a lookup into `graphResult.result` and
`graphResult.result` can be null in certain circumstances (e.g. when the
action fails), we needed to guard against this (since we don't want an
internal error for such cases).
github-merge-queue bot pushed a commit that referenced this pull request Feb 20, 2025
This should fix a regression introduced in
#6850.

Because we added a lookup into `graphResult.result` and
`graphResult.result` can be null in certain circumstances (e.g. when the
action fails), we needed to guard against this (since we don't want an
internal error for such cases).
@stefreak stefreak mentioned this pull request Feb 21, 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.

2 participants