Add outputs for the changes data by laughedelic · Pull Request #707 · actions/dependency-review-action

laughedelic · 2024-03-02T04:21:20Z

Hi! I'm using this action in conjunction with others and it would be very useful for the subsequent steps to be able to access the data that this action presents in the summary, but in a more structured format. So I added 4 outputs: one with all changes data and 3 optional ones with the data on vulnerable dependencies, invalid licenses and denied dependencies. Here's an example usage:

      - name: Dependency review
        uses: actions/dependency-review-action@v4
        id: dependency-review
        with:
          # ...

      - name: Render dependencies lineage
        uses: laughedelic/gha-dependency-lineage@v0
        if: failure()
        with:
          vulnerable-dependencies: ${{ steps.dependency-review.outputs.vulnerable-changes }}

The second step is a custom action that analyses dependencies lineage and renders a graph diagram in the job summary. ~~Without these added outputs, an action like that doesn't have any other way to know which dependencies are relevant for the report.~~ (I guess, it's possible to call the API directly, but it's wasteful when the information is already available from this action and it would only provide the unfiltered information).

I would appreciate any feedback and suggestions.

laughedelic · 2024-03-02T17:13:40Z

One thing to consider here is that action outputs have a size limit:

Outputs are Unicode strings, and can be a maximum of 1 MB. The total of all outputs in a workflow run can be a maximum of 50 MB.

I don't know how big the dependencies JSON can get. I think 1MB of JSON is a lot 🤔

febuiles · 2024-03-04T09:06:47Z

@laughedelic thanks for the contribution. Before folks start reviewing this, can I ask you to:

Add examples of the usage to https://github.com/actions/dependency-review-action/blob/main/docs/examples.md
Add unit tests for new features.
Link to a sample PR in a custom repository running your version of the Action.

@jonjanego Would appreciate your thoughts on this feature. Also, we should probably add ☝️ to CONTRIBUTING.md, or maybe a PR template?

jonjanego · 2024-03-04T16:36:21Z

Thank you @laughedelic ! I like the idea, but agree with @febuiles that we'd like to see some more examples and documentation.

I assume these are all optional parameters, and they're created as standard output objects to the rest of the workflow?

As far as the limits on JSON goes, it's a good thing we should document but agree that 1MB of JSON is quite large

laughedelic · 2024-03-04T22:41:25Z

Hi @febuiles @jonjanego, thanks for the feedback!

Add examples of the usage to https://github.com/actions/dependency-review-action/blob/main/docs/examples.md

Done in 75be7f0

Add unit tests for new features.

I'm not sure how action outputs can be tested in unit tests, since they are not like outputs of a function. But I've modified the .github/workflows/dependency-review.yml workflow (84b80e6) to print out the outputs with some minimal checks: comment-content shouldn't be empty and the rest should pass as valid JSON through jq. Let me know if this is good enough as a test, or if not, please, advise on how to test it better.

Link to a sample PR in a custom repository running your version of the Action.

Here is an example PR: laughedelic#7 which adds a dependency with a known vulnerability. And here are the outputs from the modified workflow: https://github.com/laughedelic/dependency-review-action/actions/runs/8147727423/job/22269075026#step:5:7

I assume these are all optional parameters, and they're created as standard output objects to the rest of the workflow?

Yes, these are standard action outputs, like the comment-content which was already there.

As far as the limits on JSON goes, it's a good thing we should document but agree that 1MB of JSON is quite large

I also added this to the docs in 05fcfa4.

jonjanego · 2024-03-06T17:06:48Z

Thank you for the contributions @laughedelic . We'll take a look at it sometime in the next couple of weeks and let you know any questions that we may have!

elireisman · 2024-03-13T20:47:12Z

👋 hello - small update on this.

First - I like the change, thanks for contributing!

Second, I am running some tests on this Action and branch using a test repo, and will get back to you soon with feedback or a PR approval. I appreciate your patience in the meantime 🙇

Will post an update ASAP

laughedelic · 2024-03-14T02:06:19Z

Hi Eli! Thanks for the heads up, and no worries, take your time 🙂

Co-authored-by: Federico Builes <febuiles@github.com>

laughedelic · 2024-03-17T23:53:14Z

I'm a bit concerned that the output is a bit delicate (or at least as you say requires a mention or example in the documentation?) if it requires a particular process to pass it along safely?

I would assume the right play is to ensure the values in the JSON are escaped regardless of how it is passed around or printed, so the output itself is ready to use for downstream callers?

Do you see a way in which we could escape these results so people reading directly from the object's property don't get bitten by cases like {"foo": "a'b"} as you pointed out above?

@febuiles @elireisman I understand your concerns, the output is "delicate" indeed. But I want to make it clear that it's not specific to this action, JS/TS or JSON data, it is about the way GitHub actions work in general: how outputs are passed around and how they get interpolated in run-steps (shell-scripts).

Even the existing comment-content output which returns simple text or HTML needs such delicate treatment (especially if it is used in a shell-script), it can also contain quotes or any other characters that may break Bash syntax or even get accidentally interpreted ($). This is true for any "large" or multiline output data.

I think it's also a bit misleading that the only examples I'm providing here are just reading outputs in shell-script steps. My real use case is to pass them as inputs to another TypeScript action which will parse JSON and do something with the data.

So I see these alternatives here:

the approach of this PR: to return data in the output directly and document the way it can be used, maybe warn how it shouldn't be used
an alternative is to write all data on disk in some predictable location:
- there can be a default (outside of the working dir) and some ways to override it, e.g. with an env var or an action input
- it can be an random place in temp + an output that provides that path. But this approach is bad for potential caching, it's better to provide a location that consumers can control.
another approach would be to set an env var with the data directly (without outputs), but I think it's weird for arbitrary JSON data and potentially "pollutes" environment for the subsequent steps

Personally, I think that the first approach is fine since the "usage hygiene" is not specific to this action. The second approach is a bit more "heavyweight", but also works and is hard to "misuse".

Let me know what you think and how we should proceed with this

febuiles · 2024-03-18T05:01:56Z

@laughedelic Thanks for your patience, and for taking the time to write all of this down. The approach in this PR makes more sense than the listed alternatives, and I'm happy to move forward with it. Please revert any unnecessary changes to .github/workflows/dependency-review.yml (I'm thinking about the "tests") and I'll try to get this released by Wednesday.

If string sanitizing is indeed a problem, users will complain and we can learn from actual examples instead of coming up with bash one liners :)

laughedelic · 2024-03-20T00:06:30Z

hey @febuiles, sounds good to me 🤝

I reverted the changes to the workflow and added a note in the readme to explain the usage caveat, but let me know if you would prefer it in some other form

jonjanego · 2024-03-20T17:42:38Z

Thank you again for the contribution, @laughedelic !

laughedelic · 2024-03-20T23:46:24Z

My pleasure! Thanks for the thorough review and a speedy release! 🚀

…#639) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [actions/dependency-review-action](https://togithub.com/actions/dependency-review-action) | action | minor | `v4.1.3` -> `v4.2.3` | --- ### Release Notes <details> <summary>actions/dependency-review-action (actions/dependency-review-action)</summary> ### [`v4.2.3`](https://togithub.com/actions/dependency-review-action/releases/tag/v4.2.3): 4.2.3 [Compare Source](https://togithub.com/actions/dependency-review-action/compare/v4.1.3...v4.2.3) #### What's Changed - Set comment as output by [@jsoref](https://togithub.com/jsoref) in [https://github.com/actions/dependency-review-action/pull/698](https://togithub.com/actions/dependency-review-action/pull/698) - Add support for calculating OpenSSF Scorecards by [@jhutchings1](https://togithub.com/jhutchings1) in [https://github.com/actions/dependency-review-action/pull/709](https://togithub.com/actions/dependency-review-action/pull/709) - Add outputs for the changes data by [@laughedelic](https://togithub.com/laughedelic) in [https://github.com/actions/dependency-review-action/pull/707](https://togithub.com/actions/dependency-review-action/pull/707) #### New Contributors - [@jhutchings1](https://togithub.com/jhutchings1) made their first contribution in [https://github.com/actions/dependency-review-action/pull/709](https://togithub.com/actions/dependency-review-action/pull/709) - [@laughedelic](https://togithub.com/laughedelic) made their first contribution in [https://github.com/actions/dependency-review-action/pull/707](https://togithub.com/actions/dependency-review-action/pull/707) **Full Changelog**: actions/dependency-review-action@v4.1.3...v4.2.3 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/xmldom/xmldom).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

laughedelic added 3 commits March 2, 2024 04:55

feat: add action outputs for different types of changes

eecc9aa

docs: update readme

ab8c384

chore: build and package

e7aef16

laughedelic requested a review from a team as a code owner March 2, 2024 04:21

laughedelic added 3 commits March 4, 2024 18:53

clarify docs, add a usage example

75be7f0

add a note about outputs size limit

05fcfa4

add checks for the outputs

84b80e6

laughedelic mentioned this pull request Mar 4, 2024

Add outputs for the changes data laughedelic/dependency-review-action#1

Draft

jonjanego added the enhancement New feature or request label Mar 7, 2024

febuiles reviewed Mar 14, 2024

View reviewed changes

Comment thread .github/workflows/dependency-review.yml Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread docs/examples.md Outdated

Comment thread docs/examples.md Outdated

Comment thread docs/examples.md Outdated

Comment thread src/main.ts

elireisman reviewed Mar 14, 2024

View reviewed changes

Comment thread .github/workflows/dependency-review.yml Outdated

laughedelic and others added 4 commits March 15, 2024 02:32

Apply suggestions for docs

e38e634

Co-authored-by: Federico Builes <febuiles@github.com>

avoid using if: always()

bc6a1f0

Merge branch 'main' into feat/data-outputs

16bfb33

update dist

47b7acc

laughedelic added 3 commits March 19, 2024 19:48

revert changes in CI

d78d095

add clarification about output usage hygiene

218a76c

fix run syntax

6585cc5

febuiles approved these changes Mar 20, 2024

View reviewed changes

febuiles merged commit 1f6240f into actions:main Mar 20, 2024

coderabbitai Bot mentioned this pull request Apr 17, 2026

fix(ci): CR Thread Gate caller job emits 'gate / CodeRabbit Thread Check' [OMN-9032] OmniNode-ai/omnimarket#321

Merged

1 task

Conversation

laughedelic commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laughedelic commented Mar 2, 2024

Uh oh!

febuiles commented Mar 4, 2024

Uh oh!

jonjanego commented Mar 4, 2024

Uh oh!

laughedelic commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonjanego commented Mar 6, 2024

Uh oh!

elireisman commented Mar 13, 2024

Uh oh!

laughedelic commented Mar 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laughedelic commented Mar 17, 2024

Uh oh!

febuiles commented Mar 18, 2024

Uh oh!

laughedelic commented Mar 20, 2024

Uh oh!

jonjanego commented Mar 20, 2024

Uh oh!

laughedelic commented Mar 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laughedelic commented Mar 2, 2024 •

edited

Loading

laughedelic commented Mar 4, 2024 •

edited

Loading