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.

Sign up for GitHub

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

Jump to bottom

fix(backport): checkout from staging branch and ask to update if needed #732

Merged
merged 1 commit into from
Sep 24, 2023
Merged

fix(backport): checkout from staging branch and ask to update if needed #732

merged 1 commit into from
Sep 24, 2023

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Sep 22, 2023 •
edited
Loading

@codecov
Copy link

codecov bot commented Sep 22, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (6d68c99) 83.03% compared to head (e8a3095) 83.03%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #732   +/-   ##
=======================================
  Coverage   83.03%   83.03%           
=======================================
  Files          37       37           
  Lines        4178     4178           
=======================================
  Hits         3469     3469           
  Misses        709      709

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

RafaelGSS
RafaelGSS reviewed Sep 22, 2023
View reviewed changes
lib/backport_session.js Outdated Show resolved Hide resolved
@rluvaton rluvaton force-pushed the allow-syncing-staging-branch-before-backporting branch from ab01c09 to ef56f71 Compare September 22, 2023 10:39
Trott
Trott reviewed Sep 22, 2023
View reviewed changes
lib/backport_session.js
@@ -61,7 +61,7 @@ export default class BackportSession extends Session {
cli.stopSpinner(`${file} does not exist in current working tree`,
cli.SPINNER_STATUS.WARN);
continue;
};
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated style change? (I'm surprised we don't lint these files.)

Copy link
Member

Choose a reason for hiding this comment

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

(Although, yeah, that semicolon shouldn't be there. Maybe leave it and ignore my comment. But uh, future enhancement for someone: Add linting or adjust the current linting to catch that stuff automatically.)

@Trott
Copy link
Member

Trott commented Sep 22, 2023

Is it reasonable to add a test for this change? (Non-blocking but of course I'd prefer a test to not having test if it's not too onerous.)

@rluvaton
Copy link
Member Author

I totally with you on adding test but there where no test to begin with

I will try to add one...

aduh95
aduh95 previously requested changes Sep 23, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Please do not hard code 'upstream', and take the name of the upstream from the config instead.

lib/backport_session.js Outdated Show resolved Hide resolved
@rluvaton rluvaton force-pushed the allow-syncing-staging-branch-before-backporting branch from ef56f71 to e8a3095 Compare September 23, 2023 22:06
@rluvaton
Copy link
Member Author

Please do not hard code 'upstream', and take the name of the upstream from the config instead.

@aduh95 fixed

@aduh95 aduh95 merged commit be11c08 into nodejs:main Sep 24, 2023
13 checks passed
@rluvaton rluvaton deleted the allow-syncing-staging-branch-before-backporting branch September 24, 2023 09:05
@github-actions github-actions bot mentioned this pull request Sep 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott left review comments

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

update requested staging branch before backporting in git-node backport
4 participants
@rluvaton @Trott @aduh95 @RafaelGSS