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

git-node: add config option to set wait times #367

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

mmarchini
Copy link
Contributor

Add two new config options: waitTimeSingleApproval and
waitTimeMultiApproval, which can be used to set how long ncu should
wait before landing a PR with 1 approval or with 2+ approvals.

@joyeecheung
Copy link
Member

Can you add some docs? From the look of the code I assume it's in hours?

@mmarchini
Copy link
Contributor Author

Sure. What's the best place to document this? git-node.md or ncu-config.md?

@joyeecheung
Copy link
Member

It's git-node specific config so I think it should be documented there.

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #367 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
+ Coverage   75.12%   75.37%   +0.24%     
==========================================
  Files          21       21              
  Lines        1399     1405       +6     
==========================================
+ Hits         1051     1059       +8     
+ Misses        348      346       -2
Impacted Files Coverage Δ
lib/pr_checker.js 97.46% <100%> (+1.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e60bc6a...ef00a4d. Read the comment docs.

Add two new config options: `waitTimeSingleApproval` and
`waitTimeMultiApproval`, which can be used to set how long ncu should
wait before landing a PR with 1 approval or with 2+ approvals.
@mmarchini
Copy link
Contributor Author

Done

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

The option names are a bit odd to me but I cannot think of anything more natural either..

@@ -78,6 +80,14 @@ class Session {
return this.config.readme;
}

get waitTimeSingleApproval() {
return this.config.waitTimeSingleApproval;
Copy link
Member

Choose a reason for hiding this comment

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

Umm, on a second thought, why are the defaults set in the PR checker, instead of here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, that makes more sense. I'll move it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, this won't work because git node metadata doesn't use a Session. So git node land would get the defaults correctly, but git node metadata wouldn't.

We could create a MetadataSession, but it seems a bit too much. Also doesn't make sense since the idea of a session (to my understanding at least) is to save the state of a command to be resumed later.

Should we keep this here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Ah, makes sense, I forgot that the PR checker is also run by the metadata generator.

@joyeecheung joyeecheung added the Work In Progress PR's that are in progress label Oct 4, 2019
@mmarchini
Copy link
Contributor Author

mmarchini commented Dec 9, 2019

@joyeecheung do you mind landing this? I don't have commit access to this repo (I also can't remove the WIP tag)

@joyeecheung joyeecheung removed the Work In Progress PR's that are in progress label Dec 9, 2019
@targos targos merged commit d5893f3 into nodejs:master Dec 10, 2019
@targos targos mentioned this pull request Dec 13, 2019
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.

3 participants