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

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

Merged
merged 6 commits into from
Jul 9, 2024
Merged

Conversation

rickycodes
Copy link
Contributor

closes #58

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

@rickycodes rickycodes requested a review from a team as a code owner May 31, 2024 14:27
@rickycodes rickycodes requested a review from Mrtenz May 31, 2024 14:27
@rickycodes
Copy link
Contributor Author

hmm, realizing I might need to approach this differently

@mcmire
Copy link
Contributor

mcmire commented May 31, 2024

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?

@rickycodes rickycodes force-pushed the issue-58 branch 3 times, most recently from f5c1bfc to c1f5049 Compare June 24, 2024 14:14
@rickycodes rickycodes requested a review from Gudahtt June 25, 2024 00:17
@mcmire
Copy link
Contributor

mcmire commented Jun 25, 2024

@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
Copy link
Member

Gudahtt commented Jun 25, 2024

Is it just a matter of removing the -x flag here?


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
Copy link
Contributor

mcmire commented Jun 25, 2024

@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
Copy link
Contributor Author

@mcmire let me know if this works!

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM.

@Mrtenz Mrtenz removed their request for review July 4, 2024 09:12
@rickycodes rickycodes merged commit 32d300a into main Jul 9, 2024
12 checks passed
@rickycodes rickycodes deleted the issue-58 branch July 9, 2024 15:17
Mrtenz added a commit that referenced this pull request Jul 11, 2024
Mrtenz added a commit that referenced this pull request Jul 11, 2024
* 5.2.0

* update changelog

* Add changelog entry for #93

* Add changelog entry for #90

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only show packages in the dry run that need publishing
4 participants