feat: rewrite to pure js #82
Conversation
There was a problem hiding this comment.
Thank you, @bxb100, for creating this; it's really great!
One suggestion I have is to add a CI check to verify that dist/core_bg.wasm and dist/index.js haven't been changed. I understand they are generated by js tools, and I trust all of you (@bxb100 and 1Password team members). However, it always makes me a bit concerned that someone might change the dist files by mistake. By adding CI checks, we ensure that the dist files are exactly what we expect to see and haven't been altered.
|
NOTE to other reviewers: This PR is large due to the generated |
|
@Xuanwo Thank you for the review. I have learned a lot from your comments about the CI check. I have added a new workflow, |
Thank you, mostly LGTM now! |
|
Inviting @edif2008 and @SimonBarendse for a review. |
|
Hi folks, really appreciate your efforts on this! ❤️ 🙌 Definitely aligned with the direction we'd like to move in for this and other integrations. We've build SDKs exactly because we saw such large desire from all of you to build integrations, and friction from doing so with CLI (e.g. performance, support for self-hosted runners, Windows support and it removes complexity from the implementation, making it easier to maintain and less error-prone). Excited to see you already help us move in that direction! 🚀 A couple things we'll want to look at to get this merged and deployed:
|
|
Thank you @SimonBarendse for the bravo comments! @bxb100 led the entire effort, so I will leave the final decision to him on how to implement changes. I'm here to explain why we need For every The entry point is defined here: load-secrets-action/action.yml Lines 14 to 16 in 0a30992 Additional documentation can be found here: https://docs.github.com/en/actions/sharing-automations/creating-actions/creating-a-javascript-action |
|
I am not sure if there are some dependencies in 1Password SDK that require |
I'm guessing 1Password has written their logic in Rust, with the JS SDK being just a wrapper. |
Yes, this is exactly it. We're using WASM to share a single implementation (in Rust) between all languages we support SDKs in. Right now that's Go, Python and JS, and we've built it with the intention to grow to more languages (in a scalable & sustainable way). |
|
Thanks for the explanation on why |
|
update 11/23: force update with minimal changes. the original version lives in branch #82-bak Thank you @Xuanwo, @SimonBarendse, and @yafanasiev for your reviews and discussions. I would like to provide some explanations and background information regarding the submitted content in this PR:
Footnotes |
Hi @SimonBarendse, I suggest we avoid taking this direction since switching from CLI-based to JS-based does introduce a breaking change. I can foresee multiple ways this action could fail. It's common and expected for GitHub Actions users to upgrade actions between major versions (every Node version bump will trigger one). Adopting I am willing to create a migration guide for users to follow if there are any break happened.
Hi @bxb100, I understand the concern from @SimonBarendse's side. To make the PR review smoother, do you think it would be a good idea to simply remove all files except for |
Thank you @bxb100! 🙌 This is very helpful. Followed up in separate issues
Rewrite specific changes - Discussing in this PRBackwards Compatibility
@Xuanwo could you help me see these too? I'm currently under the impression that the new version of this action built on SDK will be fully compatible with the current version built on CLI, after addition of Auth Prompts. As @bxb100 mentioned as well:
Could you please help me see and understand what I'm missing? What are the failures you foresee? DistYes, I'm less concerned about this now and okay with proceeding with checking in the WASM binary into the code here after @Xuanwo's explanation why this is necessary. Apologies that I not explicitly mentioned that before. I assume and hope when using the action that GitHub only fetches the referenced commit and not the full Git history, similar to the checkout action, by using So the larger repo size from historic binaries checked in would only affect new contributors checking out the full repo, including all historic WASM binaries. But it won't affect using the action. With that in mind, I believe we can pull back in the changes to Code ReviewI've requested a review from @Marton6 and @edif2008 for the specific code changes made in this PR. |
|
Thank you, @SimonBarendse, for the detailed explanation.
I believe we can proceed after #83 #84 .
base on https://github.com/actions/runner/blob/078eb3b381939ee6665f545234e1dca5ed07da84/src/Runner.Worker/ActionManager.cs#L784C1-L792C1 I think it direct download like |
There was a problem hiding this comment.
Hey @bxb100!
First of all, thank you for doing all of this rewrite. I really appreciate it and it looks great so far.
A couple of pieces that I wanted to raise here:
-
Backwards compatibility
I've re-read the code for the current version of this GitHub Action and the desktop app integration has never been an authentication method supported by this action. It will error if neither of the connect nor service account environment variables are configured. This should, therefore, smoothen this migration to a pure JS action and significantly decrease the risk of this not being backwards compatible. -
Windows support
Since with this change we can (finally) add support for Windows runners, would you mind adding this OS in thetest.ymlfile in theosmatrix? It should look like this:strategy: matrix: os: [ubuntu-latest, macos-latest, windows-latest] auth: [connect, service-account] exclude: - os: macos-latest auth: connect - os: windows-latest auth: connect
-
Code review
The overall approach of the changes in this PR look really good. A couple of them are nitpicks and improvements in naming and such, but a couple of key areas to look at are:- erroring when duplicate fields are found.
- identifying environment variables that have secret references and validating them.
- simplifying the code for validating the authentication and building the client that will resolve the secret references.
Co-authored-by: Eduard Filip <[email protected]>
- throw error when multiple fields found
- move `ref_regex` test to `util.test.ts`
Co-authored-by: Eduard Filip <[email protected]>
There was a problem hiding this comment.
Functional review: ❌
I've run this version of the action on happy and unhappy paths.
One regression with this implementation is that the following scenario now fails in both Connect and service account authentications:
An item with two fields secret label twice: one which isn't in any sections, and one that is in a section. Their secret references would look as follows:
- Field in no section:
op://vault/item/my-secret - Field in section:
op://vault/item/section/my-secret
Both should return the value. However, in this rewrite, the first one will fail with the following error:
error resolving secret reference: more than one field matched the secret reference
We've noticed that for the SDK, that is a bug and we're working on fixing it. For the Connect client implementation, that can be addressed as part of this PR.
Code review: ✅
This looks even better. I like the direction of this rewrite. The last step is formatting the code and making it lint-compliant.
Other notes
Windows support
It seems that testing the pipeline against the Windows runners unveiled that this migration is not fully ready for Windows support. Two key pieces that are currently blocking this:
- Configuration action: This piece is currently written in shell script, which is not recognized in Windows runners.
- Pipeline configuration: Integration tests run shell scripts, which need to be rewritten in the case of Windows runners.
Therefore, to not increase the complexity of this PR, I'd like to ask you to remove the Windows runner from the pipeline and we will address these in a separate PR. For simplicity, reverting bad9d5c will suffice. Apologies for the inconvenience.
Running tests pipeline
It seems that the test pipeline didn't execute on your branch. I have #87 open which should address this.
Linting
Unfortunately, the lint pipeline doesn't verify for this, and we will address it with #88.
In the meantime, I'm working on improving the linter so that you can also check for any issues on your PR and address them accordingly (as part of #83).
| import { OPConnect } from "@1password/connect/dist/lib/op-connect"; | ||
| import { FullItem } from "@1password/connect/dist/model/fullItem"; | ||
|
|
||
| describe("test connect with different secret refs", () => { |
There was a problem hiding this comment.
One test worth adding is getting a field that is in a section, but is unique in the item like so: op://vault/item/field. This one should also succeed in getting the field.
There was a problem hiding this comment.
Item JSON
{
"overview": {
"title": "GitHub Action Test Bak",
"ainfo": ""
},
"details": {
"notesPlain": "",
"sections": [
{
"name": "2cvyorng3vnw6li2xnmgr2bvsa",
"title": "section",
"fields": [
{
"t": "text",
"n": "pke3kfordlgadlk5ezjxfgpxra",
"k": "string",
"v": "`section/text`",
"a": {
"multiline": "yes"
},
"inputTraits": {
"autocorrection": "no",
"autocapitalization": "Sentences"
}
}
]
},
{
"name": "4jng3nm7yzsijlrs5u47law3fi",
"title": "add more",
"fields": [
{
"t": "text",
"n": "ldzsoyrn54ntzm3xb6m5lhedoi",
"k": "string",
"v": "`add more/text`",
"a": {
"multiline": "yes"
},
"inputTraits": {
"autocorrection": "no",
"autocapitalization": "Sentences"
}
},
{
"t": "cs",
"n": "z3fmurrywq3hreerkejaxtx4ai",
"k": "string",
"v": "`add more/cs`",
"a": {
"multiline": "yes"
},
"inputTraits": {
"autocorrection": "no",
"autocapitalization": "Sentences"
}
}
]
},
{
"name": "add more",
"title": "",
"fields": [
{
"t": "text",
"n": "ft5nzrsp3p6xsezeffiyizb7hu",
"k": "string",
"v": "hello world",
"a": {
"multiline": "yes"
},
"inputTraits": {
"autocorrection": "no",
"autocapitalization": "Sentences"
}
}
]
}
]
},
"createdAt": "2024-11-05T14:54:49Z",
"updatedAt": "2024-12-17T04:54:09Z",
"faveIndex": 0,
"trashed": "N",
"templateUuid": "003",
"uuid": "5vtojw237m2cxymfkfhtxsrazm"
}Item Screeshot
- CLI:
op --version2.30.3 - SDK:
@1password/sdk0.1.5
| secret_reference | SDK | CLI |
|---|---|---|
"op://dev/GitHub Action Test Bak/section/text" |
section/text |
section/text |
"op://dev/GitHub Action Test Bak/add more/text" |
❌resolving secret reference: more than one section matched the secret reference | ❌could not read secret 'op://dev/GitHub Action Test Bak/add more/text': The item has more than one "add more.text" field. Include the section name to specify which field to modify: ' |
"op://dev/GitHub Action Test Bak/add more/cs" |
❌resolving secret reference: more than one section matched the secret reference | add more/cs |
"op://dev/GitHub Action Test Bak/text" |
❌resolving secret reference: more than one field matched the secret reference | hello world |
"op://dev/GitHub Action Test Bak/cs" |
add more/cs |
add more/cs |
Note
section named add more on purpose
bit confused about:
"op://dev/GitHub Action Test Bak/section/text"work but"op://dev/GitHub Action Test Bak/add more/text"error in CLI"op://dev/GitHub Action Test Bak/cs"and"op://dev/GitHub Action Test Bak/text"both work in CLI
There was a problem hiding this comment.
according spec op://<vault-name>/<item-name>/[section-name/]<field-name>1(section-name is optional), I think using
"op://dev/GitHub Action Test Bak/section/text""op://dev/GitHub Action Test Bak/add more/text""op://dev/GitHub Action Test Bak/text"
should return an error to introduce the user using unique UUID
Footnotes
|
Hey @bxb100,
These features are actively being scoped and prioritized for future updates to the SDK. |
Hi, @edif2008, thank you so much for your review! This idea aligns with my initial thoughts, so here’s my +1 support. I fully support moving quickly rather than causing delays here. |
|
I have no objections, agree with @Xuanwo |
|
Hey @bxb100! Just wanted to let you know that I've merged a couple of improvements related to the pipelines, as well as the ES Lint tooling. If you could merge the latest version of |
# Conflicts: # package-lock.json # src/utils.test.ts
|
Done, PTAL @edif2008 |
| for (const envName of envs) { | ||
| extractSecret(envName, shouldExportEnv); | ||
| for (const key of refs) { | ||
| await extractSecret(auth, key, shouldExportEnv); |
There was a problem hiding this comment.
Hi, I'm guessing it's not a blocker, right? This PR is quite large now, how about move all non-blocker changes to separate PRs instead?
as mentioned in #80, this is a PR for rewriting to js
PS. I can't pass the default lint config, so I choose to use the GitHub action template eslint.yml
I tested Service Accounts and 1Password Connect locally using
npm run local-action. If any reviewer is interested, you can test it locally by copying.env.exampleto.env.