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

chore(git-node): Add make lint-md reminder #883

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ruyadorno
Copy link
Member

@ruyadorno ruyadorno commented Jan 8, 2025

After two consecutive releases in which I mistakenly pushed a broken release commit to main, I'm convinced that the process needs improvement.

This PR proposes to add a reminder to git node release promote to lint markdown files prior to pushing branches to upstream remote.

Refs: nodejs/node@a344300
Refs: nodejs/node#56508

After two consecutive releases in which I mistakely pushed a broken
release commit to main, I'm convinced that the process needs
improvement.

This changeset proposes to add a reminder to `git node release promote`
to lint markdown files prior to pushing branches to upstream remote.

Refs: nodejs/node@a344300
Refs: nodejs/node#56508
@ruyadorno ruyadorno force-pushed the add-reminder-to-lint-release-commit-on-main branch from ba67a3c to f2ad232 Compare January 8, 2025 15:55
@ruyadorno ruyadorno requested review from aduh95 and RafaelGSS January 8, 2025 15:56
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.08%. Comparing base (4bd6820) to head (f2ad232).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #883   +/-   ##
=======================================
  Coverage   80.08%   80.08%           
=======================================
  Files          39       39           
  Lines        4676     4676           
=======================================
  Hits         3745     3745           
  Misses        931      931           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +198 to +202
cli.separator();
cli.warn('Reminder: Lint markdown files');
cli.info('You might need to amend the commit if there are issues:');
cli.info('make lint-md');
cli.separator();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes more sense to move that to the branch where we know there were conflicts (line 168), for the cases where there are no conflicts, it seems safe to skip it

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my original assessment but in the specific occurrence linked in the PR body the bad formatting was introduced by a git automatic resolution, meaning that in theory a successfully cherry-picked commit could potentially still introduce markdown issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe it, if I try locally:

git reset a344300bfa146fce9d1f20a71b073263a4720eab^ --hard # rewind just before the faulty commit
git cherry-pick v22.13.0^0 # cherry-pick the release commit

All 7 of the YAML sections fixed by nodejs/node@5119049 show up as conflict that must be manually resolved. There might be something else that "does it for you" (like an IDE extension or whatnot), but I'm quite confident git would never

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.

4 participants