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

Removes connected_port, implements connected_ports #2076

Merged
merged 1 commit into from Feb 27, 2020

Conversation

@drews256
Copy link
Contributor

drews256 commented Nov 19, 2019

Description

This work starts the effort of removing connected_port.

Referencing this issue:
#1996

I have moved the code around a bit to get all connected_ports, the tests seem
to pass in the appropriate areas.

I want to verify this path before attempting to remove all UniquePort calls in
the tests.

@nateberkopec Also, I wanna talk about removing UniquePort, as it seems like it is being used for some CLI specs and the control-url. Is that approriate? Do we expect to be able to pass 0 in as the port for the control and be able to get it's connected port?

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] to all commit messages.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
This work starts the effort of removing connected_port from the specs.

I have moved the code around a bit to get all connected_ports, the tests seem
to pass in the appropriate areas.

I want to verify this path before attempting to remove all UniquePort calls in
the tests.
@drews256 drews256 force-pushed the drews256:removes-connected-port branch from a2cdc0b to c4f6ac0 Nov 19, 2019
@nateberkopec
Copy link
Member

nateberkopec commented Nov 26, 2019

Do we expect to be able to pass 0 in as the port for the control and be able to get it's connected port?

Control servers should be able to "bind" to port 0, just like app servers, yes.

@nateberkopec nateberkopec linked an issue that may be closed by this pull request Feb 27, 2020
0 of 2 tasks complete
end

attr_reader :connected_port
attr_reader :connected_ports

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Feb 27, 2020

Member

Not necessary as you define the method on the next line

@nateberkopec nateberkopec merged commit d91c112 into puma:master Feb 27, 2020
11 of 12 checks passed
11 of 12 checks passed
build build
Details
OS: ubuntu-16.04 Ruby: 2.3.x
Details
OS: ubuntu-18.04 Ruby: 2.4.x
Details
OS: ubuntu-18.04 Ruby: 2.5.x
Details
OS: ubuntu-18.04 Ruby: 2.6.x
Details
OS: macos Ruby: 2.4.x
Details
OS: macos Ruby: 2.5.x
Details
OS: macos Ruby: 2.6.x
Details
OS: windows-latest Ruby: 2.4.x
Details
OS: windows-latest Ruby: 2.5.x
Details
OS: windows-latest Ruby: 2.6.x
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
nateberkopec added a commit that referenced this pull request Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.