search

MetaMask / action-npm-publish

GitHub Action to publish to NPM
MIT License
8 stars 6 forks source link

Only show packages in the dry run that need publishing #90

Closed rickycodes closed 3 months ago

rickycodes commented 5 months ago

closes #58

this will also ensure reports are only generated for packages that have changes

rickycodes commented 5 months ago

hmm, realizing I might need to approach this differently

mcmire commented 5 months ago

Yeah, my thought was that instead of running through all packages here https://github.com/MetaMask/action-npm-publish/blob/main/scripts/main.sh#L11 we could figure out the packages whose versions have changed ahead of time and then just iterate over those. Would that work?

mcmire commented 4 months ago

@rickycodes I took a look at the checks that were run on this PR, and I am not sure this solves the pain point that I outlined in the original ticket.

The problem is not necessarily that we attempt to "dry run" publish packages that haven't changed. The problem is that packages that haven't changed still show up in the job output. For instance, in this PR, only @metamask/queued-request-controller is being published. As a member of NPM publishers, though, if I'm reviewing this PR I see this job (open up "Dry Run Publish"). Notice that all of the packages are being iterated over here I end up having to mentally skip everything except for @metamask/queued-request-controller. Certainly your changes here will reduce how much output we see per package, but core has a lot of packages, and we anticipate adding more in the future. So I think if we can only run publish.sh for packages that have changed, then the dry run output would be easier to review.

What do you think?

Gudahtt commented 4 months ago

Is it just a matter of removing the -x flag here? https://github.com/MetaMask/action-npm-publish/blob/7cf06c22c4d130c8a21ddc5a48ed8fb46e283f1a/scripts/main.sh#L3 That seems to account for most of the output at the moment. And we could reduce output further by omitting a message in the case where we decide a package can be skipped.

With those gone, it should be 1-2 lines per package, which seems more than good enough. I think we'd need to loop through each workspace twice to get extraneous output to zero (and omit the --verbose flag from yarn workspaces foreach on the first pass).

mcmire commented 4 months ago

@Gudahtt Sure, if we could remove -x I think that would help too. Perhaps we could only enable it if debug logging is turned on (which ends up setting the RUNNER_DEBUG environment variable, I believe?). Then, we can iterate on this further in the future if need be.

@rickycodes What are your thoughts on that?

rickycodes commented 4 months ago

@mcmire let me know if this works!