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

The trustedExtensionAuthAccess env VSCODE_TRUSTED_EXTENSIONS is Case Sensitive #368

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

satyamburhade
Copy link

What does this PR do?

Makes the trustedExtensionAuthAccess env VSCODE_TRUSTED_EXTENSIONS Case-Insensitive

What issues does this PR fix?

https://issues.redhat.com/browse/CRW-6573

How to test this PR?

Use trustedExtensionAuthAccess feature by adding VSCODE_TRUSTED_EXTENSIONS env through ConfigMap or through devfile, the value should contain lower and uppercase letters.

Copy link

github-actions bot commented Jun 6, 2024

Click here to review and test in web IDE: Contribute

@RomanNikitenko
Copy link
Contributor

@satyamburhade
thank you for your contribution!

Unfortunately, it's not possible to apply PR Checks if a PR is created from a fork
please see eclipse-che/che#22660
cc @vitaliy-guliy

@satyamburhade
I can move your changes to a branch of the main repo and open a PR if you don't mind

@vitaliy-guliy
Copy link
Contributor

@satyamburhade I can move your changes to a branch of the main repo and open a PR if you don't mind

Yes, we definitely need to move the changes to a branch on the main repository.
But I think as well the code needs to be fixed a bit because of

#30 8.732 [error] tests/trusted-extensions.spec.ts: SyntaxError: '}' expected. (274:1)
#30 8.732 [error]   272 |   });
#30 8.732 [error]   273 | });
#30 8.732 [error] > 274 |

@RomanNikitenko I think we need to review/revert/rework the github actions to allow PR checks on the branches that have been made from a fork. It will encourage more guys to improve our product by easily creating a PR and sure testing the changes (we need to describe that in CONTRIBUTING.md).

@RomanNikitenko
Copy link
Contributor

@RomanNikitenko I think we need to review/revert/rework the github actions to allow PR checks on the branches that have been made from a fork. It will encourage more guys to improve our product by easily creating a PR and sure testing the changes (we need to describe that in CONTRIBUTING.md).

I proposed it here eclipse-che/che#22660 (comment)

@vitaliy-guliy
Copy link
Contributor

@RomanNikitenko I think we need to review/revert/rework the github actions to allow PR checks on the branches that have been made from a fork. It will encourage more guys to improve our product by easily creating a PR and sure testing the changes (we need to describe that in CONTRIBUTING.md).

I proposed it here eclipse-che/che#22660 (comment)

At that time we had issues with building che-dev image.
So for pull requests created from a fork I would create a separate GH action and would omit building of che-dev image.

@vitaliy-guliy
Copy link
Contributor

@satyamburhade please update/complete the PR
you can test the changes locally by launching yarn in the launcher directory

@satyamburhade
Copy link
Author

Hello @vitaliy-guliy
Sure, will check it locally and update asap

@satyamburhade
Copy link
Author

Hello @vitaliy-guliy
I checked the code locally, found some issues and fixed them.

All tests are successful now for tests/trusted-extensions.spec.ts
Screenshot 2024-06-15 at 1 01 30 AM

fixed code style with prettier command
Copy link

@RomanNikitenko
Copy link
Contributor

@satyamburhade
I tested the use case when VSCODE_TRUSTED_EXTENSIONS env variable contains lower case value for two extensions.
The result is:

  • Manage Trusted Extensions For Account command displays both extensions as trusted ones
  • I am able to use both extensions - they work well for me

So, it looks like VS Code works well with the lower case values.

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Jun 25, 2024

@satyamburhade
I tested the use case when the VSCODE_TRUSTED_EXTENSIONS env variable contains capital letters.
For such case I had a problem with an extension.
Probably we need someone who could test it and confirm the result.

image

@RomanNikitenko
Copy link
Contributor

@azatsarynnyy
could you test the changes?

please use Clear side data from the browser console to remove cached data before testing

@RomanNikitenko
Copy link
Contributor

Tested today one more time, but, unfortunately, the same result:

image

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I've tested the patch by running a workspace from a test repository provided by Roman https://github.com/RomanNikitenko/web-nodejs-sample/tree/testPR

Manage Trusted Extensions For Account command shows:
image

Both extensions are installed:
image

Chat view:
image


BTW, I see a different set of environment variables in the terminals, depending on how it was opened: by the New Terminal command or New Terminal (Select a Container)
image

Do you guys know if it's a known issue?

@RomanNikitenko
Copy link
Contributor

@azatsarynnyy
thank you for the testing!
it looks like you faced the same problem, correct?

about

I've tested the patch by running a workspace from a test repository provided by Roman https://github.com/RomanNikitenko/web-nodejs-sample/tree/testPR

sorry, but I didn't provide the repo for testing, I provided a reference for lines with VSCODE_TRUSTED_EXTENSIONS variable - just as an example.
I use that repo very actively for testing different things - it can be changed few times a day - so you can get unexpected result when you use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants