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

Revert "test: mark test-cluster-bind-privileged-port flaky on arm" #36884

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 12, 2021

This reverts commit a45a404.

Solved by marking ports <1024 as privileged on Docker containers.

Ref: #36850
Ref: #36847
Ref: nodejs/build#2521

/cc @nodejs/build

This reverts commit a45a404.

Solved by marking ports <1024 as privileged on Docker containers.

Ref: nodejs#36850
Ref: nodejs#36847
Ref: nodejs/build#2521
@rvagg rvagg requested a review from jasnell January 12, 2021 02:47
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 12, 2021
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 12, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2021
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if as a next step, the test should be modified to allow for either no error or a check that the expected error appears and is propagated. Then the test would at least work no matter where it was run and we wouldn't have to maintain privileged port configuration on Build hosts.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

aduh95 pushed a commit that referenced this pull request Jan 18, 2021
This reverts commit a45a404.

Solved by marking ports <1024 as privileged on Docker containers.

Ref: #36850
Ref: #36847
Ref: nodejs/build#2521

PR-URL: #36884
Refs: #36850
Refs: #36847
Refs: nodejs/build#2521
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ash Cripps <acripps@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@aduh95
Copy link
Contributor

aduh95 commented Jan 18, 2021

Landed in 5716130

@aduh95 aduh95 closed this Jan 18, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
This reverts commit a45a404.

Solved by marking ports <1024 as privileged on Docker containers.

Ref: #36850
Ref: #36847
Ref: nodejs/build#2521

PR-URL: #36884
Refs: #36850
Refs: #36847
Refs: nodejs/build#2521
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ash Cripps <acripps@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.