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

Making single port 8000 default and update NetworkErrorBoundary #3656

Merged
merged 5 commits into from
Jun 9, 2021

Conversation

MatheusdiPaula
Copy link
Contributor

What

It solves #3647

How

I just followed the tasks described

  • set API_URL=/api/v1/ in the default .env files and kube manifests
  • set 502 response codes as a reason for showing the network boundary error
  • update docs

Pre-merge Checklist

  • [ x] Run integration tests
  • Publish Docker images

Recommended reading order

  1. .env
  2. .env.dev
  3. kube/overlays/stable/.env
  4. kube/overlays/dev/.env
  5. NetworkErrorBoundary.tsx

Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

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

Wow this was super fast! Thanks!

Two things:

  1. Can you also modify the port forwarding instructions in docs/deploying-airbyte/*?
  2. Unfortunately we're blocked on merging this until the error handling is fixed. Waiting to see what's necessary from Artem.

@@ -14,9 +14,9 @@ class NetworkErrorBoundary extends React.Component<
static getDerivedStateFromError(error: {
message: string;
status?: number;
}): { unReachServer: boolean } {
}): { unReachServer: boolean, status: number } {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this section needed to read more like:

  static getDerivedStateFromError(error: {
    message: string;
    status?: number;
  }): { unReachServer: boolean } {
    // Update state so the next render will show the fallback UI.
    return {
      unReachServer:
        error.message === "Failed to fetch" ||
        (error.status !== undefined && error.status === 502),
    };
  }

It's trying to set the unReachServer boolean to whether or not it should show the "server is loading error". We want to show that error when there's an error message that contains "Failed to fetch" or if the status code is set and the status code is 502 (which means the underlying service we're proxying through isn't available yet).

However, both versions (the one in this PR and the version above), I got a Uncaught SyntaxError: Unexpected token < in JSON at position 0. I believe this is because /api/v1/workspaces/get is getting an HTML An error occurred 502 page from nginx, which is not JSON.

@jamakase what's the best way to handle this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In PR #3438 I totally refactored how we handle errors and introduced a richer error hierarchy. You could merge it as is and I will update it to the same format.

It also makes parsing error's response optional, so it won't fail if the error returned is not json.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's great.

I think that since this won't actually show the "server is loading" error until #3438 it probably makes sense to wait until #3438 is merged, merge the changes into this PR and then merge the PR? Otherwise first time users will be confused about the behavior like before we added the "server is loading" error in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrhizor sounds reasonable to me. I need to address few comments for that PR and after that we could merge. Should I merge master into this branch as soon as we are ready for merging?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that'd be perfect.

@cgardens cgardens requested a review from jamakase May 27, 2021 16:27
@MatheusdiPaula
Copy link
Contributor Author

Wow this was super fast! Thanks!

Two things:

  1. Can you also modify the port forwarding instructions in docs/deploying-airbyte/*?
  2. Unfortunately we're blocked on merging this until the error handling is fixed. Waiting to see what's necessary from Artem.
  1. Done!

@mjmanas0699
Copy link

working on load balancer as well in k8s
API_URL=/api/v1/

Thanks! @MatheusdiPaula

@marcosmarxm
Copy link
Member

@MatheusdiPaula could you update your branch and resolve the conflicts?

@jrhizor
Copy link
Contributor

jrhizor commented Jun 4, 2021

To resolve the conflict you should be able to just remove airbyte-webapp/src/components/NetworkErrorBoundary/NetworkErrorBoundary.tsx since it's been replaced by ApiErrorBoundary.tsx and no changes are needed to that file.

As soon as that's updated here I'll do a final end to end test locally before merging.

# Conflicts:
#	airbyte-webapp/src/components/NetworkErrorBoundary/NetworkErrorBoundary.tsx
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 8, 2021
@MatheusdiPaula
Copy link
Contributor Author

@MatheusdiPaula could you update your branch and resolve the conflicts?

Branch updated!

@jrhizor
Copy link
Contributor

jrhizor commented Jun 8, 2021

Thanks @MatheusdiPaula!

I'll make sure we get this in by EOD tomorrow.

@jrhizor
Copy link
Contributor

jrhizor commented Jun 9, 2021

Thanks so much for this @MatheusdiPaula!

I'm going to merge a followup after this but this looks good to go!

@jrhizor jrhizor merged commit e4335b1 into airbytehq:master Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants