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

fix: improve port checks #97

Merged
merged 8 commits into from
Sep 3, 2024
Merged

fix: improve port checks #97

merged 8 commits into from
Sep 3, 2024

Conversation

abuchanan-airbyte
Copy link
Collaborator

This PR is doing a few things related to improving the port checks (portAvailable and getPort):

  • getPort checks that the container status is running and returns an error otherwise
  • portAvailable is only called when installing a new cluster (no existing cluster)
  • portAvailable a nicer error message by looking for "address in use" error codes.
  • portAvailable doesn't try to guess whether an existing port is used by Airbyte or not (since it's now only called when no cluster exists)

Some code-level changes:

  • I moved some docker mock test stuff into dockertest
  • I merged docker.Port() into getPort() - it was only being called from getPort and with it merged, I can easily do the status == running check, and the tests seem improved as well.
  • getPort() now takes a cluster name, instead of the whole k8s.Provider

I made these changes to address to scenarios:

  1. I have another, unrelated container bound to port 8000
  2. I have an abctl cluster that was created but the container has been stopped (e.g. I restarted my Docker Desktop)

These changes improved the experience in these cases:

Unrelated container bound to port 8000

Before:
Screenshot 2024-08-30 at 1 29 31 PM

After:
Screenshot 2024-08-30 at 1 25 06 PM

Cluster in stopped container

Before:
Screenshot 2024-08-30 at 1 32 56 PM

After:
Screenshot 2024-08-30 at 1 53 43 PM

@abuchanan-airbyte abuchanan-airbyte requested a review from a team as a code owner August 30, 2024 21:08
@abuchanan-airbyte
Copy link
Collaborator Author

I think this broke something with the spinners

if ipPort.HostIP == "0.0.0.0" {
port, err := strconv.Atoi(ipPort.HostPort)
if err != nil {
return 0, InvalidPortError{ipPort.HostPort, err}
Copy link
Member

Choose a reason for hiding this comment

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

Should we include the keys here as well?

@abuchanan-airbyte abuchanan-airbyte merged commit 1825378 into main Sep 3, 2024
2 checks passed
@abuchanan-airbyte abuchanan-airbyte deleted the ab/port-check branch September 3, 2024 18:19
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.

2 participants