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.

Sign up for GitHub

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

Jump to bottom

Apply Secrets plugin API #9463

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Apply Secrets plugin API #9463

merged 1 commit into from
Jun 8, 2021

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented May 11, 2021 •
edited by vince-fugnitto
Loading

Signed-off-by: Igor Vinokur [email protected]

What it does

Corresponding CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23332.
Apply Secrets plugin API: https://code.visualstudio.com/updates/v1_53#_secrets-api

fixes #9348

How to test

  1. Clone, compile and start the sample project: https://github.com/vinokurig/secrets-test.
  2. Run Get Password command and see a notification with empty (undefined) password.
  3. Run Set Password command and see a notification about password change.
  4. Run Get Password command and see a notification with the updated password.
  5. Run Delete Password command and see a notification about password change.
  6. Run Get Password command and see a notification with empty (undefined) password.

Review checklist

Reminder for reviewers

azatsarynnyy
azatsarynnyy approved these changes May 13, 2021
View reviewed changes
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.

Tested it with the provided extension - works as expected.

packages/core/src/node/keytar-server.ts Outdated Show resolved Hide resolved
vince-fugnitto
vince-fugnitto reviewed May 14, 2021
View reviewed changes
packages/core/package.json
@@ -34,6 +34,7 @@
"ajv": "^6.5.3",
"body-parser": "^1.17.2",
"cookie": "^0.4.0",
"keytar": "7.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

I confirmed that the keytar dependency is license compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to create a CQ for this package?

Copy link
Member

Choose a reason for hiding this comment

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

Do I need to create a CQ for this package?

@vinokurig I don't think so, it falls under the following use-case. I confirmed that the dependency looks fine, you may double-check if you like following this process.

packages/core/src/browser/credentials-service.ts
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/
// code copied and modified from https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/services/credentials/common/credentials.ts#L12
Copy link
Member

@vince-fugnitto vince-fugnitto May 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We likely need a CQ for the two files, can you add a link to the corresponding CQ when ready (unless it is already covered)?

https://github.com/microsoft/vscode/blob/1.55.2/src/vs/workbench/services/credentials/common/credentials.ts#L12
https://github.com/microsoft/vscode/blob/1.55.2/src/vs/platform/native/electron-main/nativeHostMainService.ts#L679-L771

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23332

@vince-fugnitto vince-fugnitto added CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) vscode issues related to VSCode compatibility labels May 14, 2021
@vinokurig vinokurig force-pushed the theia-9348 branch 2 times, most recently from b981597 to 777dd19 Compare May 18, 2021 08:02
@vinokurig
Copy link
Contributor Author

@vince-fugnitto is it OK to merge?

@vince-fugnitto
Copy link
Member

vince-fugnitto commented May 19, 2021 •
edited
Loading

@vince-fugnitto is it OK to merge?

@vinokurig No? The CQ is not yet approved.

@vinokurig vinokurig mentioned this pull request May 20, 2021
Closed
@vinokurig
Copy link
Contributor Author

@vince-fugnitto The CQ has been approved is there any other objections?

@vince-fugnitto
Copy link
Member

@vince-fugnitto The CQ has been approved is there any other objections?

@vinokurig the lat comment I see is:

This CQ is currently awaiting PMC Approval...

Is it somehow not refreshed?

@vinokurig
Copy link
Contributor Author

vinokurig commented May 27, 2021 •
edited by vince-fugnitto
Loading

@vince-fugnitto Does this mean that it's approved?

@vince-fugnitto
Copy link
Member

@vince-fugnitto Does this mean that it's approved?

@vinokurig I've always waited for there to be an explicit approval comment as part of the CQ, namely:

We have completed a high level triage of the attachment and you may now check the content into your project’s source code repository.

@vinokurig
Copy link
Contributor Author

@vince-fugnitto

Does this mean that it's approved?

Sorry, forgot to paste my screenshot:
screenshot-dev eclipse org-2021 05 27-17_17_12

I've always waited for there to be an explicit approval comment as part of the CQ, namely:

We have completed a high level triage of the attachment and you may now check the content into your project’s source code repository.

OK, let's wait for the comment.

@vinokurig vinokurig force-pushed the theia-9348 branch 3 times, most recently from 5ab76ed to 1542bb1 Compare June 7, 2021 14:44
@vinokurig
Copy link
Contributor Author

@vince-fugnitto since the CQ has been approved I am going to merge it tomorrow, if now other objections?

@vinokurig vinokurig force-pushed the theia-9348 branch 2 times, most recently from 43030c9 to a9df031 Compare June 8, 2021 05:42
@vinokurig
Apply Secrets plugin API
1dbd97c 
Signed-off-by: Igor Vinokur <[email protected]>
@vinokurig vinokurig merged commit f9c0311 into eclipse-theia:master Jun 8, 2021
@colin-grant-work colin-grant-work mentioned this pull request Jun 10, 2021
Closed
@colin-grant-work colin-grant-work mentioned this pull request Jun 24, 2021
Closed
@colin-grant-work colin-grant-work mentioned this pull request Jul 2, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto left review comments

@azatsarynnyy azatsarynnyy azatsarynnyy approved these changes

@paul-marechal paul-marechal Awaiting requested review from paul-marechal

@marcdumais-work marcdumais-work Awaiting requested review from marcdumais-work

@RomanNikitenko RomanNikitenko Awaiting requested review from RomanNikitenko

@vzhukovs vzhukovs Awaiting requested review from vzhukovs

Assignees
No one assigned
Labels
CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) vscode issues related to VSCode compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement the Secrets API
3 participants
@vinokurig @vince-fugnitto @azatsarynnyy