Add outputs for the changes data

[laughedelic]

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]

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]

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

1. Add examples of the usage to https://github.com/actions/dependency-review-action/blob/main/docs/examples.md
2. Add unit tests for new features.
3. 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]

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]

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]

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]

👋 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]

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

[laughedelic]

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]

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

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]

Thank you again for the contribution, @laughedelic !

[laughedelic]

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