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

Save the full diff as a file #116

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

TWiStErRob
Copy link
Contributor

This is useful to be able to easily upload the inputs and the diff as an artifact.
We could do a manual diff, but it's simpler to do it here as it's already calculated.

@mateuszkwiecinski mateuszkwiecinski merged commit edafd75 into usefulness:master Feb 16, 2024
2 of 4 checks passed
@mateuszkwiecinski
Copy link
Member

Thanks! Am I right to assume it makes some sense to expose the file path as an action output, to make it more explicit? The suggested approach someone finds this file can be consumed later on, right? 👀

@mateuszkwiecinski
Copy link
Member

Okay, that's what I did in #117
The file can be now be easily referenced and consumed by i.e. actions/upload-artifact:

      - uses: actions/upload-artifact@v4
        with:
          name: dependency-tree-diff-output
          path: ${{ steps.dependency-diff.outputs.file-diff }}

@TWiStErRob TWiStErRob deleted the patch-1 branch February 16, 2024 21:54
@TWiStErRob
Copy link
Contributor Author

@mateuszkwiecinski we actually wanted to upload all 3 files (only 2 found, 3rd in this PR), because it might help to look at the full context if there's something strange in the diff. Having all 3 available from CI means there's no need to get the branch and run the command, saving a bit of time + the CI environment might be configured differently so it could also help debug that.

Exposing the outputs might make sense, consider doing them all then. You might want to change the file names (which might break some), but but old_diff and new_diff is a bit misleading, because those are not diffs, just the old and new dependency dumps.

@mateuszkwiecinski
Copy link
Member

Thanks for more context, it totally makes sense 👍 Sooo, my proposal (merged already to the main branch) is:

     - uses: actions/upload-artifact@v4
       with:
          name: dependency-tree-diff-output
          path: |
              ${{ steps.dependency-diff.outputs.dependencies-previous }}
              ${{ steps.dependency-diff.outputs.dependencies-current }}
              ${{ steps.dependency-diff.outputs.file-diff }}

They should point at files named:

  • dependency_tree_diff_output.txt
  • dependency_tree_diff_dependencies_previous.txt
  • dependency_tree_diff_dependencies_current.txt

(although, I consider these names an implementation detail, so I wouldn't rely on them.)

I'm not fully satisfied with the names (dependencies-previous & dependencies-current), but I couldn't figure out anything fitting better (some of alternatives I considered: dependencies-this-ref/...-base-ref or dependencies-old/...-new).
I'll wait a bit in case you have any feedback and if everything seems to be fine I'll publish a regular release

@TWiStErRob
Copy link
Contributor Author

For consistency consider:

              ${{ steps.dependency-diff.outputs.file-dependencies-previous }}
              ${{ steps.dependency-diff.outputs.file-dependencies-current }}
              ${{ steps.dependency-diff.outputs.file-diff }}

also to disambiguate between dependency_tree diff_output and dependency_tree_diff output, I would recommend:

dependency-tree-diff_output.txt
dependency-tree-diff_dependencies-previous.txt
dependency-tree-diff_dependencies-current.txt

re old/new prev/curr this/base, when I wrote a similar tool, I used base/head following PR naming vernacular. I think anything works though, it's pretty clear which one is which in all cases. previous/current has a benefit that the two words are not the same length, which could be interpreted as a bad thing, but might also help find the one you want faster.

although, I consider these names an implementation detail, so I wouldn't rely on them.
if everything seems to be fine I'll publish a regular release

Consider that since there wasn't a public API (step output) before this, people might've hardcoded the old file names, and you could break them. (We'll definitely need to adjust our usage.)

@mateuszkwiecinski
Copy link
Member

Thanks for all the feedback 🙇 All the changes should be included with 2.1.0 release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants