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

golang: refresh dask-gateway-proxy using modules and package directories #559

Merged
merged 15 commits into from
Jun 6, 2022

Conversation

rigzba21
Copy link
Contributor

@rigzba21 rigzba21 commented Apr 21, 2022

@consideRatio Restructuring dask-gateway-proxy using go project layout to address #506

Nothing has changed as far as functionality.

  • Both router.go and sni.go have been moved into separate packages under the pkg/ directory.
  • main.go has been moved to cmd/dask-gateway-proxy.
  • logging.go has been moved to its own internal package under internal/logging.
  • Generated a new go.mod and go.sum with dependencies.
  • GitHub Actions updated to point to pkg/, cmd/dask-gateway-proxy/, and internal/.
    • Currently there are only unit tests for pkg/router.

I hope this helps!


EDIT: closes #506!

rigzba21 and others added 12 commits April 21, 2022 15:48
…-proxy

Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
@rigzba21 rigzba21 marked this pull request as ready for review May 3, 2022 02:50
Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
@rigzba21 rigzba21 changed the title restructuring dask-gateway-proxy using modules and package directories golang: refresh dask-gateway-proxy using modules and package directories May 3, 2022
Copy link
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Thank you for your work on this @rigzba21!!! From what I can tell, this looks good. I left a few comments for my own insight but this LGTM from what I can tell.

Also note that the test failures are unrelated at least to this PR (ipython/traitlets#731).

Signed-off-by: rigzba21 <jonathan.velando01@gmail.com>
@consideRatio
Copy link
Collaborator

@martindurant @TomAugspurger @jacobtomlinson a ping for review, i figure we merge this soonish without further comments.

@TomAugspurger
Copy link
Member

I've never written go before, but +1 to the goals of this PR.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This seems like a great move to me. The only comment I have is why are we putting some code under pkg? I don't think we should be intentionally exposing any public APIs and we should probably place it in internal instead.

@rigzba21
Copy link
Contributor Author

rigzba21 commented May 31, 2022

The only comment I have is why are we putting some code under pkg? I don't think we should be intentionally exposing any public APIs and we should probably place it in internal instead.

I thought about that, however, I figured that might be a design decision outside of the scope of this PR, and might fit better in a separate PR, and I do not have as much context into the long-term goals for dask-gateway-proxy.

@consideRatio
Copy link
Collaborator

I figured that might be a design decision outside of the scope of this PR, and might fit better in a separate PR

Deal! Let's go for a merge on this. The tests were passing before we ran into an issue I think was related to traitlets (ipython/traitlets#731).

@consideRatio
Copy link
Collaborator

@rigzba21 thank you so much for your work on this!!!!!! 🎉 ❤️ 🌻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh dask-gateway-server/dask-gateway-proxy with modern golang practices
4 participants