Skip to content

Update VS Code References (settings sync schema, leeway) #8077

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

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

filiptronicek
Copy link
Member

@filiptronicek filiptronicek commented Feb 7, 2022

Description

This PR does two things:

  1. It updates the Settings Sync types and schema, including the pointer to VS Code's code base
    a. Specifically, user tasks can be synced with this PR
  2. It migrates the old name of our VS Code repo -> vscode to the new one -> openvscode-server

Related Issue(s)

Follow-up to #8048

How to test

Make sure user tasks are synced correctly. In order to do that, do the following:

  1. Open a workspace in the preview environment, for instance the Node.js example.
  2. Open the command pallete (F1) and execute Tasks: Open User Tasks
  3. A JSON file will open in your editor with a default config, we need to alter this config, here is an example one:
    {
        "version": "2.0.0",
        "tasks": [
            {
                "label": "Test sync",
                "type": "shell",
                "command": "echo 'Hewo Gitpod (ᵘʷᵘ)'",
            }
        ]
    }
  4. Open another workspace and repeat step two; the task should be synced in your new workspace and executable via Tasks: Run Task (your task will be listed first). Then confirm by selecting Continue without scanning the task output.

Release Notes

NONE

@filiptronicek filiptronicek self-assigned this Feb 7, 2022
@filiptronicek filiptronicek requested review from a team February 7, 2022 21:43
@github-actions github-actions bot added team: IDE team: webapp Issue belongs to the WebApp team labels Feb 7, 2022
@filiptronicek
Copy link
Member Author

/hold

@akosyakov
Copy link
Member

I guess how to test section should explain how to define a user task which can be shared and how to verify that it is shared?

@akosyakov
Copy link
Member

@filiptronicek should it be opened for review. why is it still a draft?

@iQQBot
Copy link
Contributor

iQQBot commented Feb 9, 2022

Because build is failed, still need change code

@roboquat roboquat added size/L and removed size/S labels Feb 10, 2022
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #8077 (93ce82b) into main (b1c83db) will increase coverage by 2.44%.
The diff coverage is n/a.

❗ Current head 93ce82b differs from pull request most recent head 234a82f. Consider uploading reports for the commit 234a82f to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##            main    #8077      +/-   ##
=========================================
+ Coverage   8.41%   10.86%   +2.44%     
=========================================
  Files         33       18      -15     
  Lines       2340     1022    -1318     
=========================================
- Hits         197      111      -86     
+ Misses      2138      909    -1229     
+ Partials       5        2       -3     
Flag Coverage Δ
components-gitpod-cli-app 10.86% <ø> (-0.32%) ⬇️
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
install-installer-raw-app ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/gitpod-cli/cmd/credential-helper.go 5.58% <0.00%> (-1.09%) ⬇️
...l/installer/pkg/components/ws-manager/tlssecret.go
install/installer/pkg/common/storage.go
install/installer/pkg/common/render.go
components/local-app/pkg/auth/pkce.go
...staller/pkg/components/ws-manager/networkpolicy.go
...nstall/installer/pkg/components/ws-manager/role.go
install/installer/pkg/common/common.go
.../installer/pkg/components/ws-manager/deployment.go
install/installer/pkg/common/ca.go
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1c83db...234a82f. Read the comment docs.

@loujaybee loujaybee added the meta: blocked in progress but blocked by upstream issues or missing data label Feb 15, 2022
@loujaybee
Copy link
Member

loujaybee commented Feb 15, 2022

Adding blocked label, some input required from @jeanp413 to help out @filiptronicek to get this one merged. Please sync, and let us know if there's any other support you need 🙏 .

@filiptronicek
Copy link
Member Author

filiptronicek commented Feb 21, 2022

Today I dug through what the ref property stands for.

This is an example object of the manifest during a settings sync:

{
    "session": "401d5491-c3bd-4605-9175-ce33aaf8c9d5",
    "latest": {
        "extensions": "371d597e-e82e-41ed-ac89-6f497bca8872",
        "globalState": "43754c45-e198-4783-8a13-cc87d670c44c",
        "machines": "d14884d9-0f6d-48a6-90b9-664db8e9a240",
        "settings": "34459307-8332-4b50-a04e-756557358f0d"
    },
    "ref": "W/\"10a-egzfJAhstbSergamP5WP7tpzSsI\""
}

Aaaah, found its origin, it's an ETag!
This is where it is generated: https://github.com/gitpod-io/openvscode-server/blob/d0b3ff8c29a181aa62d09461df5f755f479f6d2f/src/vs/server/node/webClientServer.ts#L54
This is where it is appended to the data: https://github.com/gitpod-io/openvscode-server/blob/4a130c40ed876644ed8af2943809d08221375408/src/vs/platform/userDataSync/common/userDataSyncStoreService.ts#L293

@jeanp413
Copy link
Member

jeanp413 commented Feb 21, 2022

Good find, here's where it's used
But the one who generates it is the settings sync server, looking at the code here it seems it gets generated automatically by express but it's a weak etag, we need to change it to be strong seems microsoft also uses weak etags and check if it's the same just respond with 304 (maybe express does this too automatically)
We don't need to add ref to IUserDataManifest here in the server as it's calculated on the fly

@filiptronicek
Copy link
Member Author

filiptronicek commented Feb 21, 2022

@jeanp413 it looks like we already indeed serve weak etags with the requests (as indicated by the etag starting with W/ [1]), thanks for noting the fact we don't have to have it on the server, I will remove it from there.
image

Thet etag is added precisely here (so not express, but our code):

@roboquat roboquat added size/M and removed size/XXL labels Feb 21, 2022
@filiptronicek filiptronicek marked this pull request as ready for review February 21, 2022 22:18
@jeanp413
Copy link
Member

jeanp413 commented Feb 22, 2022

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-ft-update-vscode-refs.12

@filiptronicek filiptronicek removed the meta: blocked in progress but blocked by upstream issues or missing data label Feb 22, 2022
@jeanp413 jeanp413 self-requested a review February 22, 2022 17:47
jeanp413
jeanp413 previously approved these changes Feb 22, 2022
Copy link
Member

@jeanp413 jeanp413 left a comment

Choose a reason for hiding this comment

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

LGTM

Remove Theia assignments

Co-authored-by: Jean Pierre <[email protected]>
@jeanp413 jeanp413 self-requested a review February 24, 2022 19:09
@akosyakov
Copy link
Member

/unhold

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM

@roboquat roboquat merged commit 0d05dcf into main Mar 1, 2022
@roboquat roboquat deleted the ft/update-vscode-refs branch March 1, 2022 13:52
@roboquat roboquat added the deployed: IDE IDE change is running in production label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production release-note-none size/M team: IDE team: webapp Issue belongs to the WebApp team
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants