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

[codehelper] replace product.json only #14821

Merged
merged 1 commit into from
Nov 21, 2022
Merged

[codehelper] replace product.json only #14821

merged 1 commit into from
Nov 21, 2022

Conversation

mustard-mh
Copy link
Contributor

@mustard-mh mustard-mh commented Nov 21, 2022

Description

Replace product.json only for browser code frontend to save duration of ide ready

See also internal chat

Related Issue(s)

Fixes #

How to test

  • Open workspace in prev env with latest code
  • Check if everything related to extension gallery is working well
    • extension search / install all to with our openvsx-proxy
    • extension detail page click to vote star and marketplace will jump to open-vsx.org without ask to trust URL

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-hw-code-prod.1 because the annotations in the pull request description changed
(with .werft/ from main)

@mustard-mh mustard-mh marked this pull request as ready for review November 21, 2022 14:34
@mustard-mh mustard-mh requested a review from a team November 21, 2022 14:34
@mustard-mh
Copy link
Contributor Author

mustard-mh commented Nov 21, 2022

Image

Tested with search, vote, marketplace. Are any other test cases to do? cc @akosyakov

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

it seems to work fine.

@roboquat roboquat merged commit 6824a8f into main Nov 21, 2022
@roboquat roboquat deleted the hw/code-prod branch November 21, 2022 14:50
@mustard-mh
Copy link
Contributor Author

Will deploy after #14825 merge

Comment on lines +281 to +282
b = bytes.ReplaceAll(b, []byte("{{extensionsGalleryItemUrl}}"), []byte("https://open-vsx.org/vscode/item"))
b = bytes.ReplaceAll(b, []byte("{{trustedDomain}}"), []byte("https://open-vsx.org"))
Copy link
Member

Choose a reason for hiding this comment

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

@mustard-mh I have a dumb question, why this got added 😅? I see they are added in dockerfile and then replaced here and in blobserve and they are just harcoded constant values 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't remember, maybe just a dumb action I did. 😅

Copy link
Member

Choose a reason for hiding this comment

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

@akosyakov @iQQBot do you remember?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like can be removed.

Copy link
Contributor Author

@mustard-mh mustard-mh Nov 21, 2022

Choose a reason for hiding this comment

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

I remembered what happened, https://open-vsx.org ({{trustedDomain}}) is too normal to replace in startup.sh, so I need to replace it to something more special, in case it is replaced by our openvsx-proxy in startup.sh.

Looks like we still need it?

Copy link
Member

Choose a reason for hiding this comment

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

But the final value is harcoded here and in blobserve, where is openvsx-proxy updating this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will replace them

b = bytes.ReplaceAll(b, []byte("https://open-vsx.org"), []byte(registryUrl))

And installer will replace https://open-vsx.org again

Copy link
Member

@jeanp413 jeanp413 Nov 21, 2022

Choose a reason for hiding this comment

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

yeah proxy url is fine, that needs to be replaced and that's why the first grep was needed.

I'm referring to {{extensionsGalleryItemUrl}}, {{trustedDomain}} they are replaced with constant values, the same values that are already in product.json in gp-code/main branch, it doesn't change like VSX_REGISTRY_URL that changes for each SH installation.

So just remove them then?

Copy link
Contributor Author

@mustard-mh mustard-mh Nov 21, 2022

Choose a reason for hiding this comment

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

The constant value are https://open-vsx.org and https://open-vsx.org/vscode/item they all contains open-vsx.org which will be replaced in installer and codehelper (startup.sh).

But we don't want them to be changed.

  • In codehelper, we can use sjson to Set value with json path we want
  • In installer, it's just global replacement

So we still need them because of installer

Copy link
Member

Choose a reason for hiding this comment

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

Ah I get it now 💡 , but then can we make the search more specific (with a regex maybe) so it doesn't replace the other values, right now it's not obvious at all

@mustard-mh mustard-mh mentioned this pull request Nov 21, 2022
5 tasks
@roboquat roboquat added deployed: IDE IDE change is running in production deployed Change is completely running in production labels Nov 21, 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 deployed Change is completely running in production release-note-none size/M team: IDE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants