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

feat: check Actions and handle doc-only changes #469

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

mmarchini
Copy link
Contributor

Doc-only changes don't need a full Jenkins CI, instead we can check
if the last Actions run was successful. Therefore this commit also adds
check for Action runs. Jenkins CI messages were improved as well.

Fix: #324
Ref: nodejs/node#32335

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #469 into master will increase coverage by 0.54%.
The diff coverage is 97.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   82.59%   83.13%   +0.54%     
==========================================
  Files          34       34              
  Lines        1660     1696      +36     
==========================================
+ Hits         1371     1410      +39     
+ Misses        289      286       -3     
Impacted Files Coverage Δ
lib/ci/ci_type_parser.js 81.35% <ø> (ø)
lib/pr_checker.js 97.14% <97.61%> (+1.65%) ⬆️

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 17ea885...2445dbf. Read the comment docs.

@mmarchini
Copy link
Contributor Author

Waiting for #468 before landing this. Also need to add testing for the new scenarions introduced and change name of some existing tests to reflect their current results.

@mmarchini
Copy link
Contributor Author

cc @nodejs/node-core-utils

@@ -25,8 +25,8 @@ const CI_TYPE_ENUM = {
};

const CI_PROVIDERS = {
JENKINS: 'jenkins',
Copy link
Member

Choose a reason for hiding this comment

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

what motivated the name change? i'm not sure it'd be clear to me what "nodejs" CI meant 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't test only jenkins anymore, so a new type made more sense. And since the only repo we use ncu and Jenkins is nodejs/node, I thought a provider specific for that repo made sense. Happy to hear other suggestions for names though.

An alternative I thought was accepting a list of providers, so for nodejs/node we would have ['jenkins', 'actions']. But that increases complexity.

Although now I realized that maybe libuv also uses ncu? They definitely generate metadata and have a similar landing process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also FWIW, we stopped using ncu on llnode, which is what motivated me to have more than one provider here. We can probably get rid of this.

@mmarchini
Copy link
Contributor Author

cc @nodejs/libuv do you use node-core-utils to land Pull Requests in libuv/libuv? Trying to determine if these changes will impact other projects.

@cjihrig
Copy link

cjihrig commented Aug 17, 2020

cc @nodejs/libuv do you use node-core-utils to land Pull Requests in libuv/libuv? Trying to determine if these changes will impact other projects.

I don't. I'd like to, but never got around to trying to make it work. I don't believe others use it either, but I don't want to speak for them.

@richardlau
Copy link
Member

I don't think node-core-utils can land anything outside of nodejs/node without workarounds? For example it assumes certain things are in the project's README such as a TSC section and email addresses.

In the past I have used git node metadata pointed at the Node.js README to generate the "Reviewed-by:" metadata for libuv commits, but landed manually.

@mmarchini
Copy link
Contributor Author

I don't think node-core-utils can land anything outside of nodejs/node without workarounds

I made it mostly work for llnode (had to send a few PRs here and use Node.js README, which is inaccurate for llnode), but lately I've been thinking it's better to keep this tool focused on nodejs/node instead of making it configurable for any project.

@mmarchini
Copy link
Contributor Author

cc @nodejs/node-core-utils

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

lgtm pending conflict resolution!

Doc-only changes don't need a full Jenkins CI, instead we can check
if the last Actions run was successful. Therefore this commit also adds
check for Action runs. Jenkins CI messages were improved as well.

Fix: nodejs#324
Fix: nodejs/node#32335
Fix: nodejs/node#29770
@mmarchini mmarchini merged commit 855f1d4 into nodejs:master Aug 20, 2020
@mmarchini mmarchini deleted the check-multiple-cis branch August 20, 2020 16:56
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.

git-node: detect CI status also from the GitHub API
5 participants