-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Remove checkoutLocation from StartWorkspaceReq #9007
Conversation
@csweichel I have fixed the outstanding issues. |
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.
LGTM
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 is a good example where the change could be across multiple PRs.
- Just proto changes and generated code
- The updated logic
Change works according to tests specified.
result.setBackup(new FromBackupInitializer()); | ||
const backup = new FromBackupInitializer(); | ||
if (CommitContext.is(context)) { | ||
backup.setCheckoutLocation(context.checkoutLocation || ""); |
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.
Shouldn't we default to "."? : 🤔
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.
Being picky here bc we had an incident after the last change in the multirepo PR
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.
Shouldn't we default to "."?
Also in all cases, even if not a CommitContext
?
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.
@csweichel Could you double-check here?
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.
@AlexTugarev This could be the problematic lines, actually. ☝️
Hi! Was this change tested with a restart of a workspace? |
Description
This PR removes the redundant
checkoutLocation
field from theStartWorkspace
request. Checkout location is a concept that exists on the content initializer, not on the workspace itself.Related Issue(s)
Fixes #7370
Related #8888
How to test
Release Notes