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

doc: add sri to cdn script tag in readme #1498

Merged
merged 6 commits into from
Mar 18, 2024
Merged

doc: add sri to cdn script tag in readme #1498

merged 6 commits into from
Mar 18, 2024

Conversation

shuowu-okta
Copy link
Contributor

No description provided.

@shuowu-okta shuowu-okta force-pushed the sw-add-sri-to-readme-OKTA-700807 branch from d024275 to bad9383 Compare March 8, 2024 20:39
lesterchoi-okta
lesterchoi-okta previously approved these changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@lesterchoi-okta lesterchoi-okta left a comment

Choose a reason for hiding this comment

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

LGTM, up to you w.r.t. CLI API

README.md Show resolved Hide resolved
scripts/generate-sri.js Outdated
const argv = yargs(hideBin(process.argv)).argv;
const version = argv['authjs-version'];
if (!version) {
throw new Error('--authjs-version is required');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: similar comment on mono PR re; CLI args vs flags = required vs optional

jaredperreault-okta
jaredperreault-okta requested changes Mar 11, 2024
View reviewed changes
scripts/generate-sri.js Outdated
const fs = require('fs');
const path = require('path');
const { spawn } = require('child_process');
const { https } = require('follow-redirects');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be accomplished without requiring an additional dependency?

scripts/generate-sri.js Outdated
}

const artifactPath = path.join(ARTIFACTS, artifact);
await unpackArchive(artifactPath, UNPACK_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

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

The fetching of the tar and unpacking could be more easily with
npm i --prefix <dir> @okta/[email protected]

scripts/generate-sri.js Outdated
}

async function main() {
const argv = yargs(hideBin(process.argv)).argv;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think parsing argv for a single flag justifies introducing a new dependency

@shuowu-okta
Copy link
Contributor Author

Compared files from CDN and registry against v7.4.1, both are generated same sri

script used

const fileFromCdn = fs.readFileSync(path.join(ARTIFACTS, 'okta-auth-js-7.4.1-fromCdn.min.js'), { encoding: 'utf8' });
const fileFromPkg = fs.readFileSync(path.join(ARTIFACTS, 'okta-auth-js-7.4.1-fromPkg.min.js'), { encoding: 'utf8' });
const sriFromCdn = generateSri(fileFromCdn);
const sriFromPkg = generateSri(fileFromPkg);
console.info('generated same sri?', sriFromCdn === sriFromPkg);

console output:

shuo.wu@F7HMVX6KDM okta-auth-js % node ./scripts/generate-sri.js
Debugger attached.
generated same sri? true

@lesterchoi-okta @jaredperreault-okta

jaredperreault-okta
jaredperreault-okta reviewed Mar 15, 2024
View reviewed changes
scripts/setup-e2e.sh Outdated
@@ -11,6 +11,7 @@ if [ -n "${TEST_SUITE_ID}" ]; then

# this chrome install is not used, however it will install linux deps chrome needs (via apt-get)
setup_service google-chrome-stable 118.0.5993.70-1
export PUPPETEER_DOWNLOAD_BASE_URL="https://storage.googleapis.com/chrome-for-testing-public"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still necessary?

lesterchoi-okta
lesterchoi-okta reviewed Mar 15, 2024
View reviewed changes
Copy link
Contributor

@lesterchoi-okta lesterchoi-okta left a comment

Choose a reason for hiding this comment

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

LGTM pending @jaredperreault-okta's comment re; PUPPETEER_DOWNLOAD_BASE_URL

@oktapp-aperture-okta oktapp-aperture-okta bot merged commit 4dc55e9 into master Mar 18, 2024
1 check passed
@oktapp-aperture-okta oktapp-aperture-okta bot deleted the sw-add-sri-to-readme-OKTA-700807 branch March 18, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesterchoi-okta lesterchoi-okta lesterchoi-okta left review comments

@jaredperreault-okta jaredperreault-okta jaredperreault-okta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shuowu-okta @lesterchoi-okta @jaredperreault-okta