-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat(apify): add decryption for input secrets #83
Conversation
# Conflicts: # package-lock.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I found just one small thing.
"playwright": "*", | ||
"puppeteer": "*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should stay here, no? They're used in the e2e tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets see if it breaks it, they are still in the lock file, and as long as we dont directly import them in the tests, they dont have to be specified here.
E2E tests for this branch running here: https://github.com/apify/apify-sdk-js/actions/runs/3203931243
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's gonna work, since they're still in the lockfile, but they're imported from the scraper source code, which is then imported dynamically through here:
apify-sdk-js/test/e2e/tools.mjs
Line 58 in 6d1af79
await import(`../../packages/actor-scraper/${scraper}/dist/main.js`); |
But I don't know how it works with the workspaces, if you reference a workspace which has them listed as dependencies, maybe you don't need to list them as your own dependencies? I guess it's fine in that case then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test passed so it should be fine.
# Conflicts: # package-lock.json
Implementation for decryption of input secrets.
I still didn't comment or document the secrets at all, I would do it once we test it internally in team and release it into public.