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.

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

merged 1 commit into from
Jan 5, 2022

Conversation

dominykas
Copy link
Member

Closes #10

@@ -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

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

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

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.

@dominykas dominykas added the enhancement New feature or request label Jan 2, 2022
Copy link
Contributor

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

I thought it may be redundant, but the detailed output can be useful to spot why a run was successful and a subsequent one fails.

@dominykas dominykas merged commit 4984865 into main Jan 5, 2022
@dominykas dominykas deleted the print-debug branch January 5, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output debug information
3 participants