[WIP] Parallelize env var reads#163
Conversation
|
Hi everyone 👋, first of all: thank you for this GitHub action! I really like the approach of storing my .env secrets in 1Password :) This PR is a PoC implementation for a performance improvement: In our application, we have about 15 environment variables that need to be loaded from 1Password. In the current implementation, this takes about 25–30 seconds. It appears that "Populating variables" is executed sequentially. For us, this is kind of unacceptable because the whole action just takes 1 minute (30 seconds of it just loading env variables). I know that this has been brought up before (#91 #101), but the referenced PR #82 has been closed. I also think that this could be fixed now without waiting for a complete rewrite. This PR is a proof-of-concept implementation of how parallel loading of environment variables might work. As a disclaimer upfront: this implementation was generated by AI. It includes some changes that I believe are useful but have nothing to do with this fix. I just wanted to know if you're interested in this change and if you think the approach is generally okay before I invest any more time in it. I tested this PR in our setup and it brought the action down to 4s. |
There was a problem hiding this comment.
Do you want the dist to be included in the PR?
| message = error.message; | ||
| } else { | ||
| String(error); | ||
| message = String(error); |
There was a problem hiding this comment.
Unrelated, but useful change. I can open a separate PR for it.
| expect(setClientInfo).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| name: "1Password GitHub Action", | ||
| id: "GHA", | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Unrelated change. I can revert that :)
| id: "GHA", | ||
| }), | ||
| ); | ||
| expect(exec.getExecOutput).toHaveBeenCalledWith("op", ["env", "ls"]); |
There was a problem hiding this comment.
Also unrelated, see my comment at the implementation
| const nodeScript = | ||
| 'const keys=JSON.parse(process.env.OP_KEYS_JSON||"[]");const out={};for(const k of keys){out[k]=process.env[k]??"";}process.stdout.write(JSON.stringify(out));'; | ||
|
|
||
| const res = await exec.getExecOutput( | ||
| "op", | ||
| [ | ||
| "run", | ||
| `--env-file=${envFile}`, | ||
| "--no-masking", | ||
| "--", | ||
| "node", | ||
| "-e", | ||
| nodeScript, | ||
| ], | ||
| { | ||
| silent: true, | ||
| env: { | ||
| ...process.env, | ||
| [envFileKeysEnvVar]: JSON.stringify(envNames), | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
I can put nodeScript into a dedicated file. Do you think the approach of "abusing" op run is ok?
| // Iterate over them to find 1Password references, extract the secret values, | ||
| // and make them available in the next steps either as step outputs or as environment variables. | ||
| const res = await exec.getExecOutput(`sh -c "op env ls"`); | ||
| const res = await exec.getExecOutput("op", ["env", "ls"]); |
There was a problem hiding this comment.
This change is also unrelated.
Personally, I think the change makes sense, but maybe there's a specific reason for sh -c? I can do a separate PR if you're interested.
1 participant
Adds support for loading secrets from an environment file in batches, improving the performance for workflows that rely on multiple secrets.
Closes #91 #101