feat: change binary download url #5369

sandor-trombitas · 2024-07-19T13:13:35Z

Pull Request Submission

Please check the boxes once done.

The pull request must:

Reviewer Documentation
- follow CONTRIBUTING rules
- be accompanied by a detailed description of the changes
- contain a risk assessment of the change (Low | Medium | High) with regards to breaking existing functionality. A change e.g. of an underlying language plugin can completely break the functionality for that language, but appearing as only a version change in the dependencies.
- highlight breaking API if applicable
- contain a link to the automatic tests that cover the updated functionality.
- contain testing instructions in case that the reviewer wants to manual verify as well, to add to the manual testing done by the author.
- link to the link to the PR for the User-facing documentation
User facing Documentation
- update any relevant documentation in gitbook by submitting a gitbook PR, and including the PR link here
- ensure that the message of the final single commit is descriptive and prefixed with either feat: or fix: , others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.
Testing
- Changes, removals and additions to functionality must be covered by acceptance / integration tests or smoke tests - either already existing ones, or new ones, created by the author of the PR.

Pull Request Review

All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.

Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.

For Node projects: It’s important to make sure changes in package.json are also affecting package-lock.json correctly.

If a dependency is not necessary, don’t add it.

When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.

Pull Request Approval

Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.

What does this PR do?

This changes the the download url for our ts-binary-wrapper. When people will install snyk-cli via npm, they will download the binary from downloads.snyk.io, if this url fails, the script will try to download it from static.snyk.io

Where should the reviewer start?

ts-binary-wrapper/src/common.ts
ts-binary-wrapper/src/bootstrap.ts

How should this be manually tested?

cd ts-binary-wrapper
npm ci --ignore-scripts
npm run build
npx ts-node ./test/util/prepareEnvironment.ts exec
mv ./test/util/something/generated/sha256sums.txt ./wrapper_dist/generated/sha256sums.txt
mv ./test/util/something/generated/version wrapper_dist/generated/version
npm run postinstall

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

github-actions · 2024-07-19T13:14:34Z

	Warnings
⚠️	Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style `import`s and `export`s to ES6 ones. A file you've modified is using either `module.exports` or `require()`. If you can, please update them to ES6 import syntax and export syntax. Files found: `ts-binary-wrapper/src/common.ts`

Generated by 🚫 dangerJS against 89546b0

ts-binary-wrapper/test/unit/common.spec.ts

j-luong · 2024-07-23T13:18:05Z

ts-binary-wrapper/src/common.ts

  public getDownloadLocation(): string {
-    const baseUrl = 'https://static.snyk.io/cli/v';
-    return baseUrl + this.version + '/' + this.binaryName;
+    const baseUrl = 'https://downloads.snyk.io/cli';
+
+    return this.buildDownloadUrl(baseUrl);
+  }
+
+  public getBackupDownloadLocation(): string {
+    const baseUrl = 'https://static.snyk.io/cli';
+
+    return this.buildDownloadUrl(baseUrl);


question: would

public getDownloadLocation(baseUrl = 'https://downloads.snyk.io/cli'): string { return baseUrl + this.version + '/' + this.binaryName; }

be cleaner? getDownloadLocation() and getBackupDownloadLocation() are more or less identical and we could get rid of buildDownloadUrl() if we just update getDownloadLocation()

PeterSchafer · 2024-07-23T17:10:27Z

ts-binary-wrapper/src/common.ts

@@ -242,7 +253,7 @@ export function downloadExecutable(
  filenameShasum: string,
 ): Promise<Error | undefined> {
  return new Promise<Error | undefined>(function(resolve) {
-    const options = new URL(downloadUrl);
+    const options = new URL(downloadUrl + '?utm_source=npm');


Praise! Happy to see this :)

Suggestion: let's change the utm_source to this something like TS_BINARY_WRAPPER to be more future proof and align with integration names.

Suggestion 2: Let's also use the chance to add the missing env variables SNYK_INTEGRATION_NAME, SNYK_INTEGRATION_VERSION to runWrapper() the integration name would also be TS_BINARY_WRAPPER for consistency.

I've added the integration name and version as env vars, let me know what you think

PeterSchafer · 2024-07-23T17:22:13Z

ts-binary-wrapper/src/common.ts

+    return this.buildDownloadUrl(baseUrl);
+  }
+
+  public getBackupDownloadLocation(): string {


Question: Would make the logic easier and more consistent if we change getDownloadLocation() to return a list of URLs?
This list could be iterated from start until one URL works. Currently there is the additional download in index that doesn't use the backup url.

ts-binary-wrapper/src/common.ts

ts-binary-wrapper/test/acceptance/basic.spec.ts

feat: add utm source search param when downloading the binary fix: prepare environment script chore: fix lint issues test: add test case for downloadWithBackup() feat: add integration env vars refactor: consolidate download locations to single method chore: cleanup chore: improve logging with timestamps chore: format and lint

sandor-trombitas force-pushed the CLI-415-binary-url branch from 326f1bf to 066fc1d Compare July 19, 2024 13:15

j-luong reviewed Jul 23, 2024

View reviewed changes

ts-binary-wrapper/test/unit/common.spec.ts Show resolved Hide resolved

j-luong reviewed Jul 23, 2024

View reviewed changes

sandor-trombitas changed the title ~~feat[deploy]: change binary download url~~ feat: change binary download url Jul 23, 2024

PeterSchafer reviewed Jul 23, 2024

View reviewed changes

j-luong reviewed Jul 26, 2024

View reviewed changes

ts-binary-wrapper/src/common.ts Outdated Show resolved Hide resolved

j-luong reviewed Jul 26, 2024

View reviewed changes

ts-binary-wrapper/test/acceptance/basic.spec.ts Outdated Show resolved Hide resolved

sandor-trombitas force-pushed the CLI-415-binary-url branch from 8705093 to 9329eed Compare August 7, 2024 08:03

sandor-trombitas marked this pull request as ready for review August 7, 2024 08:03

sandor-trombitas requested a review from a team as a code owner August 7, 2024 08:03

sandor-trombitas force-pushed the CLI-415-binary-url branch from 9329eed to 1a3d0d1 Compare August 7, 2024 08:03

j-luong approved these changes Aug 16, 2024

View reviewed changes

j-luong force-pushed the CLI-415-binary-url branch from 1a3d0d1 to 4c956e5 Compare August 16, 2024 08:25

sandor-trombitas force-pushed the CLI-415-binary-url branch 2 times, most recently from 9ab07fe to cead704 Compare August 16, 2024 11:23

sandor-trombitas enabled auto-merge August 16, 2024 11:23

sandor-trombitas force-pushed the CLI-415-binary-url branch from cead704 to c3a83d0 Compare August 16, 2024 11:48

sandor-trombitas force-pushed the CLI-415-binary-url branch from c3a83d0 to 89546b0 Compare August 16, 2024 12:27

sandor-trombitas merged commit 438337e into main Aug 16, 2024
11 checks passed

sandor-trombitas deleted the CLI-415-binary-url branch August 16, 2024 13:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change binary download url #5369

feat: change binary download url #5369

sandor-trombitas commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading

j-luong Jul 23, 2024 •

edited

Loading

PeterSchafer Jul 23, 2024

PeterSchafer Jul 23, 2024 •

edited

Loading

PeterSchafer Jul 23, 2024

sandor-trombitas Jul 26, 2024

PeterSchafer Jul 23, 2024

feat: change binary download url #5369

feat: change binary download url #5369

Conversation

sandor-trombitas commented Jul 19, 2024 • edited Loading

Pull Request Submission

Pull Request Review

Pull Request Approval

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots

Additional questions

github-actions bot commented Jul 19, 2024 • edited Loading

j-luong Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

PeterSchafer Jul 23, 2024

Choose a reason for hiding this comment

PeterSchafer Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

PeterSchafer Jul 23, 2024

Choose a reason for hiding this comment

sandor-trombitas Jul 26, 2024

Choose a reason for hiding this comment

PeterSchafer Jul 23, 2024

Choose a reason for hiding this comment

sandor-trombitas commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading

j-luong Jul 23, 2024 •

edited

Loading

PeterSchafer Jul 23, 2024 •

edited

Loading