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

Update Getting Started Guide with note on port 5000 for MacOS #1994

Merged
merged 3 commits into from
May 19, 2022
Merged

Update Getting Started Guide with note on port 5000 for MacOS #1994

merged 3 commits into from
May 19, 2022

Conversation

RNHTTR
Copy link
Contributor

@RNHTTR RNHTTR commented May 17, 2022

Problem

Port 5000 is now a reserved port on MacOS

Solution

Include note in getting started guide for how to run the API server on a custom port

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've updated the CHANGELOG.md with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary)
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)

Signed-off-by: Ryan Hatter ryannhatter@gmail.com

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1994 (dfda1eb) into main (878457b) will not change coverage.
The diff coverage is n/a.

❗ Current head dfda1eb differs from pull request most recent head 049b8f8. Consider uploading reports for the commit 049b8f8 to get more accurate results

@@            Coverage Diff            @@
##               main    #1994   +/-   ##
=========================================
  Coverage     78.23%   78.23%           
  Complexity      955      955           
=========================================
  Files           194      194           
  Lines          5293     5293           
  Branches        420      420           
=========================================
  Hits           4141     4141           
  Misses          706      706           
  Partials        446      446           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@collado-mike
Copy link
Collaborator

Per my comment in #1956 , there's not really a strong reason for us to map any of these ports to any specific host port. Only the web UI needs to be on a deterministic host port - the rest of the containers should probably rely on Docker to manage the port mapping behind the scenes

@wslulciuc
Copy link
Member

wslulciuc commented May 17, 2022

@collado-mike, we could allow Docker to manage the port, but there are two use-cases for starting the API container. One being the web UI, but let's say you want to curl the API, or use one of our clients, knowing the port upfront is helpful.

Who knew the macOS would have reserved port 5000 :/

@wslulciuc
Copy link
Member

@collado-mike
Copy link
Collaborator

One being the web UI, but let's say you want to curl the API, or use one of our clients, knowing the port upfront is helpful.

Those are fair use cases - the former can be handled using the docker port command (a little clunky, but... 🤷🏽‍♂️). Or we can also add a docker-compose override file to map to a specific port, but leave the default mapping random. We could use something like a --fixed-api-port XXXX or something to determine when to include the override, but by default everything would work on Mac or any other system (as long as the web port isn't conflicting).

RNHTTR added 2 commits May 18, 2022 19:50
Signed-off-by: Ryan Hatter <ryan.hatter@astronomer.io>
Signed-off-by: Ryan Hatter <ryan.hatter@astronomer.io>
@wslulciuc
Copy link
Member

wslulciuc commented May 19, 2022

@collado-mike: In favor of making standing up Marquez locally on a Mac easier, and avoiding the port issue, we'll have docker assign the port (see #1956). We can merge for now, but make the port conflict changes to our docker compose files as follow up work.

Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@wslulciuc wslulciuc enabled auto-merge (squash) May 19, 2022 17:59
@wslulciuc wslulciuc merged commit 64ae128 into MarquezProject:main May 19, 2022
jonathanpmoraes pushed a commit to nubank/NuMarquez that referenced this pull request Feb 6, 2025
…zProject#1994)

* Update Getting Started Guide with note on port 5000 for MacOS

Signed-off-by: Ryan Hatter <ryan.hatter@astronomer.io>

* Update README.md

Signed-off-by: Ryan Hatter <ryan.hatter@astronomer.io>

Co-authored-by: Willy Lulciuc <willy@datakin.com>
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.

3 participants