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

Print debug information #36

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Print debug information #36

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/workflows/node-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ jobs:
with:
node-version: ${{ matrix.node-version }}

- name: Print version information
run: |
echo OS: $(node -p "os.version()")
echo Node.js: $(node --version)
echo npm: $(npm --version)
echo git: $(git --version)


- name: Run post-checkout steps
if: ${{ inputs.post-checkout-steps }}
uses: ./.github/tmp/post-checkout-steps
Expand All @@ -126,6 +134,9 @@ jobs:
if: ${{ inputs.post-install-steps }}
uses: ./.github/tmp/post-install-steps

- name: Print installed dependencies
Copy link
Member

Choose a reason for hiding this comment

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

This will be a ton of output; are you sure it’s helpful?

Copy link
Member Author

@dominykas dominykas Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Does it hurt? What are the downsides?

I'm not sure, tbh, how useful this is on OSS side of things, but at work this has been a life saver in numerous occassions:

The later two can be somewhat solved by using lockfiles, but there's opinions about that.

Copy link
Member

@ljharb ljharb Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What opinions? The latter two absolutely are covered by lockfiles.

Also, GitHub actions are almost exclusively used by open source projects, so that’s what we should be optimizing for.

Copy link
Member

Choose a reason for hiding this comment

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

The downsides are that it might cause actually important log output to be truncated if the log gets too large - I’m not sure what GitHub’s log size limit is, but this specific output caused that problem on projects for me on Travis in the past, so i don’t do it anymore.

Copy link
Member Author

@dominykas dominykas Jan 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What opinions

I meant that while lockfiles cover the issues mentioned, there are opinions about using lockfiles. Esp. in the context of OSS libraries a lot of people don't use them?

not sure what GitHub’s log size limit is

https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration#artifact-and-log-retention-policy does not mention any size limits.

I do recall them recently posting a blog post about how they rewrote the log viewer to cater for larger log files 🤔 https://github.blog/2021-03-25-how-github-actions-renders-large-scale-logs/ mentions that they need to support at least 50k lines. Surely that should be enough to list the dependencies 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oh definitely there are opinions about that :-) but by not using a lockfile, I’d use --before locally to investigate anyways, which the full tree output wouldn’t help with.

Copy link
Member

Choose a reason for hiding this comment

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

how much time does it add to list and log the entire dep tree?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does say 0s here: https://github.com/dominykas/pkgjs-action/runs/4685158763?check_suite_focus=true (randome poke). Other places say 1s. Ofc that's a very small tree, but I doubt it would go much over that.

Regarding the log lines, it could be attached as an artifact, but that would probably add another second... Not sure it's worth that.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough :-) altho actually it might be much more useful to attach the lockfile itself - or, the one npm would have produced - as an artifact.

I guess i can see the usefulness of both, based on what you’ve described. The lockfile could be looked into as a follow up.

run: npm ls --all
continue-on-error: true

- name: Run tests
run: ${{ inputs.test-command }}
Expand Down