-
Notifications
You must be signed in to change notification settings - Fork 328
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
Run cloud project locally #12198
base: develop
Are you sure you want to change the base?
Run cloud project locally #12198
Conversation
🧪 Storybook is successfully deployed!📊 Dashboard:
|
5b95795
to
aebbad9
Compare
aebbad9
to
3697a86
Compare
|
||
/** Unpack a .tar.gz enso-project bundle into a temporary directory */ | ||
export async function unpackBundle(bundle: stream.Readable): Promise<string> { | ||
const ensoProjectsDirectory = |
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 it correct that in shim we don't respect the root directory setting ?
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 updated the logic. Right now we create a {projects-root}/cloud-{projectId}
directory for the cloud project. In the future, we will make it hidden. I suppose it requires using a native library for Windows.
if (projectsStore.getState().launchedProjects.length > 0) { | ||
closeAllProjects() | ||
return eventCallbacks.useEventCallback( | ||
(project: LaunchedProject & { cloudProjectId?: backendModule.ProjectId }) => { |
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.
What's the purpose of keeping cloudProjectId
property separately from the LaunchedProject
prop?
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 an extra parameter I need to pass when starting the project in the Hybrid mode. We need an original id of the cloud project to upload it after it is closed.
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 mean, why we won't add it to LaunchedProject
interface, or introduce another interface that extends the LaunchedProject
?
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.
Ok, I updated the schema
await client.resetQueries({ queryKey: createGetProjectDetailsQuery.getQueryKey(id) }) | ||
setProjectAsset(type, id, parentId, (asset) => ({ | ||
...asset, | ||
projectState: { ...asset.projectState, type: backendModule.ProjectState.closed }, | ||
})) | ||
|
||
// If the project runs in hybrid execution mode | ||
if (cloudProjectId) { |
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 case if user closes local project in offline mode, the mutation will fail because remoteBackend is not available. This might lead to unexpected behavior.
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.
Not sure how to handle the failure properly. I'll leave a TODO comment
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 think typical for dashboard isa FIXME with a link to a new issue in cloud-v2
doAction={async () => { | ||
invariant(localBackend != null, 'Local Backend is null') | ||
const parentId = await remoteBackend.downloadProject(asset.id) | ||
const assets = await localBackend.listDirectory({ |
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.
prefer ensureQueryData
over calling methods manually: https://tanstack.com/query/latest/docs/reference/QueryClient#queryclientensurequerydata
Prefer mutations over direct calls here. Also, consider moving this out of the function for better reusability
async downloadProject(id: backend.ProjectId): Promise<DirectoryId> { | ||
const details = await this.getProjectDetails(id, true) | ||
|
||
invariant(details.url != null, 'The download URL of the project must be present.') |
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.
prefer passing id directly using parameters instead.
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.
The invariant
method only silences the compiler. getPropertyDetails
should always return url
property when called with the parameter true
. It is used this way in the download
method above, for example
const response = await this.client.get(`./api/cloud/download-project?${queryString}`) | ||
const path = await response.text() | ||
|
||
invariant(response.ok, 'The download-project response must have status OK.') |
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.
As I mentioned above, the invariant
is supposed for uh-oh
situations. Prefer throwing an error here instead
directory: extractIdFromDirectoryId(directoryId), | ||
}) | ||
|
||
await this.client.get(`./api/cloud/upload-project?${queryString}`) |
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 think it's better to have /
instead of ./
here. Also we prefer url constructor functions instead of bare bone strings. Please take a look at similar methods and do it similarly
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.
It looks like this module uses strings. I wasn't able to find any examples of URL usages
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.
It was inspired by
enso/app/gui/src/dashboard/layouts/AssetContextMenu.tsx
Lines 302 to 304 in 28a1b03
const projectResponse = await fetch( | |
`./api/project-manager/projects/${localBackendModule.extractTypeAndId(asset.id).id}/enso-project`, | |
) |
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.
yeah fwiw:
- i prefer
./
so that it works when proxied with a base path (e.g. through code-server or openvscode-server)- however i guess that's not super robust - instead we could use
APP_BASE_URL
: https://github.com/enso-org/enso/blob/e002d942a5d49ec2d275fdeb8f0c3e62ebaa2906/app/gui/src/dashboard/utilities/appBaseUrl.ts
- however i guess that's not super robust - instead we could use
- dunno about url constructors here. maybe it's possible for it to work but i think this is more understandable. after all, only the path needs changing so idk if there are many advantages to using
new URL
instead (if there were query string changes involved i would absolutely recommendnew URL
though)
Can you add mocked support for calling endpoints:
|
@PabloBuchu what's the difference between |
Pull Request Description
implements https://github.com/enso-org/cloud-v2/issues/1651
This is a POC of the Hybrid execution feature https://github.com/enso-org/cloud-v2/issues/1650
Changelog:
Run Locally
button for Cloud projects that downloads the project into a temporary directory and starts it using the local backendHybrid Execution
feature that enablesRun Locally
button for Cloud projects. The feature is enabled for dev builds.enso/project.json
metadata when listing projects using theproject-manager-shim-middleware
Important Notes
Uploading logic is implemented but blocked by https://github.com/enso-org/cloud-v2/issues/1715
enso-12198-run-locally.mp4
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.