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

doc: replace deprecated CI job #21938

Closed
wants to merge 1 commit into from
Closed

doc: replace deprecated CI job #21938

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

node-test-pull-request-lite CI job is deprecated now and https://ci.nodejs.org/job/node-test-pull-request-lite/ has this note: "DEPRECATED: please use the node-test-pull-request-lite-pipeline job".

`node-test-pull-request-lite` CI job is deprecated now and
https://ci.nodejs.org/job/node-test-pull-request-lite/ has this note:
"DEPRECATED: please use the node-test-pull-request-lite-pipeline job".
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 22, 2018
@vsemozhetbyt vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Jul 22, 2018
@vsemozhetbyt
Copy link
Contributor Author

Not sure if this should be approved by the TSC or any other WG. Please, cc or label as seems appropriate.

@vsemozhetbyt
Copy link
Contributor Author

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Comment is non-blocking for me.

@@ -213,7 +213,7 @@ needs to be pointed out separately during the onboarding.
* Optionally, include your personal pronouns.
* Label your pull request with the `doc` and `notable-change` labels.
* Run CI on the PR. Because the PR does not affect any code, use the
`node-test-pull-request-lite` CI task. Alternatively, use the usual
`node-test-pull-request-lite-pipeline` CI task. Alternatively, use the usual
`node-test-pull-request` CI task and cancel it after the linter and one other
subtask have passed.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one for @nodejs/build to comment on, but my own experience with Jenkins is that cancelling jobs can sometimes leave workspaces in a bad state (usually manifesting in git checkout errors on subsequent runs) depending on where the job was at at the time it was cancelled so I'm not generally in favour of encouraging the cancellation of jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my personal experience, a lot of git checkout errors / infra failures in general with ci.nodejs.org can be linked back to issues with the machine itself, not usually due to cancelling jobs early. Also, you could say there's a benefit from new collaborators practicing how to launch node-test-pull-request jobs. I'd rather folks let the build go all the way through and finish, but I don't think they're "breaking our infra" by cancelling it early.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer that cancelling jobs is not encouraged at all, I've seen a lot of failures because of it. It may not be the most frequent issue but it happens.

At least two cases are know: 1) Abort during a git operation will leave behind a lock file that will make all following jobs on that machine fail; 2) Visual Studio will not stop running immediately after the job is aborted, locking some files and making git clean fail for some time/jobs (happens easily when there are queued jobs). Note that Jenkins does not offer us any reliable way of running anything between jobs, so fixing this is not straightforward.

(To be clear, I'm also not blocking this PR, but would be in favor of deleting the sentence starting at "Alternatively".)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make a new PR after landing to draw more attention to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR: #21977

@vsemozhetbyt
Copy link
Contributor Author

cc @nodejs/automation It seems the git-node needs updating its lite CI detection.

@vsemozhetbyt
Copy link
Contributor Author

Landed in c75f36f
Thank you for the reviews.

@vsemozhetbyt vsemozhetbyt deleted the doc-replace-deprecated-ci-job branch July 25, 2018 21:33
vsemozhetbyt added a commit that referenced this pull request Jul 25, 2018
`node-test-pull-request-lite` CI job is deprecated now and
https://ci.nodejs.org/job/node-test-pull-request-lite/ has this note:
"DEPRECATED: please use the node-test-pull-request-lite-pipeline job".

PR-URL: #21938
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: João Reis <[email protected]>
targos pushed a commit that referenced this pull request Jul 26, 2018
`node-test-pull-request-lite` CI job is deprecated now and
https://ci.nodejs.org/job/node-test-pull-request-lite/ has this note:
"DEPRECATED: please use the node-test-pull-request-lite-pipeline job".

PR-URL: #21938
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: João Reis <[email protected]>
vsemozhetbyt added a commit that referenced this pull request Jul 26, 2018
Refs: #21938 (comment)
and discussion below the comment.

PR-URL: #21977
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this pull request Jul 26, 2018
Refs: #21938 (comment)
and discussion below the comment.

PR-URL: #21977
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants