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

docs(website): Update tutorial #8737

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

docs(website): Update tutorial #8737

wants to merge 1 commit into from

Conversation

rettichschnidi
Copy link
Contributor

@rettichschnidi rettichschnidi commented Jun 8, 2024 •
edited
Loading

This commit updates the content of the tutorial, but does not change anything fundamental.

Open Questions/TODOs:

  • What are the current min versions for Node.js, NPM and Yarn?
  • Resolve all in-line questions.

@rettichschnidi rettichschnidi force-pushed the gardena/rs/upstream/update-tutorial branch from f05c9fa to b0cd522 Compare June 8, 2024 00:42
@codecov Codecov
Copy link

codecov bot commented Jun 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.85%. Comparing base (42862bf) to head (c359d20).

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #8737   +/-   ##
=========================================
  Coverage     67.85%   67.85%           
  Complexity     1165     1165           
=========================================
  Files           244      244           
  Lines          7736     7736           
  Branches        864      864           
=========================================
  Hits           5249     5249           
  Misses         2128     2128           
  Partials        359      359
Flag Coverage Δ
funTest-docker 66.84% <ø> (ø)
funTest-non-docker 34.17% <ø> (ø)
test 38.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rettichschnidi rettichschnidi changed the title docs(website): Update tutorial [RFC] docs(website): Update tutorial Jun 8, 2024
@rettichschnidi rettichschnidi changed the title [RFC] docs(website): Update tutorial docs(website): Update tutorial Jun 8, 2024
@rettichschnidi
docs(website): Update tutorial
c359d20 
This commit updates the content of the tutorial, but does not change
anything fundamental.

Signed-off-by: Reto Schneider <[email protected]>
@rettichschnidi rettichschnidi force-pushed the gardena/rs/upstream/update-tutorial branch from b0cd522 to c359d20 Compare June 8, 2024 00:53
sschuberth
sschuberth reviewed Jun 8, 2024
View reviewed changes
website/docs/getting-started/tutorial.md
@@ -65,7 +59,7 @@ For reliable results we use version 2.1.18 (replace `[mime-types-dir]` with the
```shell
git clone https://github.com/jshttp/mime-types.git [mime-types-dir]
cd [mime-types-dir]
git checkout 2.1.18
git checkout 2.1.35
Copy link
Member

@sschuberth sschuberth Jun 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Note that it might make sense to stick to the older 18 patch-level for comparability to some "mime-types-2.1.18" test assets we have in our code base, like https://github.com/oss-review-toolkit/ort/blob/main/scanner/src/test/assets/scancode-3.0.2_mime-types-2.1.18.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated because using 2.1.18 resulted in tons of issues due to outdated dependencies. Which makes it harder to read the resulting file. 2.1.35 has issues too, but (currently) only a few.

Would it be worth to update the tests assets?

Copy link
Member

Choose a reason for hiding this comment

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

I updated because using 2.1.18 resulted in tons of issues due to outdated dependencies.

What kind of issues exactly? Do you mean security vulnerabilities? However, I believe we do not even run the advisor yet as part of the tutorial... see #4219.

rettichschnidi
rettichschnidi commented Jul 26, 2024
View reviewed changes
Copy link
Contributor Author

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

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

(gosh, just realized I have never actually sent my comments/questions)

website/docs/getting-started/tutorial.md
ERROR - Resolving dependencies for 'package.json' failed with: No lockfile found in '[mime-types-dir]'. This potentially results in unstable versions of dependencies. To support this, enable the 'allowDynamicVersions' option in 'config.yml'.
Writing analyzer result to '[analyzer-output-dir]/analyzer-result.yml'.
[workdir]/mime-types
00:31:07.985 [main] WARN org.ossreviewtoolkit.plugins.packagemanagers.node.utils.NpmDetection - Any of [NPM, PNPM, YARN, YARN2] could be the package manager for '[workdir]/mime-types/package.json'. Assuming it is an NPM project.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a) Why 4x the same warning?
b) Should this be warning just be left out of the tutorial, or addressed by some other means?

Copy link
Member

Choose a reason for hiding this comment

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

Why 4x the same warning?

Because all of NPM, PNPM, YARN, YARN2 individually state via common NpmDetection code that none of them is clearly responsible for that definition file.

Should this be warning just be left out of the tutorial, or addressed by some other means?

If quoting console output, I believe it should be complete to avoid confusion about differences. Maybe the only exception should be lengthy / unimportant output that could be replaced by ellipsis ([...]).

website/docs/getting-started/tutorial.md
source: "NPM"
message: "deprecated [email protected]: Glob versions prior to v9 are no longer supported"
severity: "HINT"
dependency_graphs:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice to have an explanation here, but I do not understand this yet.

Copy link
Member

Choose a reason for hiding this comment

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

You mean the "deprecated [email protected]: Glob versions prior to v9 are no longer supported" message? That's just being forwarded from NPM itself. Apparently the project uses an unsupported / unmaintained version of the glob package.

website/docs/getting-started/tutorial.md Show resolved Hide resolved
website/docs/getting-started/tutorial.md
@@ -65,7 +59,7 @@ For reliable results we use version 2.1.18 (replace `[mime-types-dir]` with the
```shell
git clone https://github.com/jshttp/mime-types.git [mime-types-dir]
cd [mime-types-dir]
git checkout 2.1.18
git checkout 2.1.35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated because using 2.1.18 resulted in tons of issues due to outdated dependencies. Which makes it harder to read the resulting file. 2.1.35 has issues too, but (currently) only a few.

Would it be worth to update the tests assets?

@sschuberth
Copy link
Member

What are the current min versions for Node.js, NPM and Yarn?

If there are minimum versions, they are defined as part of an overridden getVersionRequirement(), see

ort/plugins/package-managers/node/src/main/kotlin/Npm.kt

Line 159 in ee94143

override fun getVersionRequirement(): RangesList = RangesListFactory.create("6.* - 10.*")

ort/plugins/package-managers/node/src/main/kotlin/Pnpm.kt

Line 66 in ee94143

override fun getVersionRequirement(): RangesList = RangesListFactory.create("5.* - 9.*")

ort/plugins/package-managers/node/src/main/kotlin/Yarn.kt

Line 73 in ee94143

override fun getVersionRequirement(): RangesList = RangesListFactory.create("1.3.* - 1.22.*")

ort/plugins/package-managers/node/src/main/kotlin/Yarn2.kt

Line 240 in ee94143

override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=2.0.0")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
2 participants
@rettichschnidi @sschuberth