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 bug on create backend to support external URL #76

Merged

Conversation

Chaho12
Copy link
Member

@Chaho12 Chaho12 commented Oct 20, 2023

I was looking at some PRs from lyft/gateway to see any improvements that can be made and noticed that externalUrl was not added on create.
If the user wanted to set external url as Gateway API guide saids, I need to create add backend, then edit external url.

There was no issue code-wise until now because proxyTo conf (a.k.a backend_url) was used instead of external_url

Also updated docs/gateway-api docs as it has typos/curl syntax issue

FYI. Test fails until #74 is merged. PR is merged and test is success

@cla-bot cla-bot bot added the cla-signed label Oct 20, 2023
@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/support_external_url branch from 8cc22b2 to 1d6b8bf Compare October 24, 2023 11:05
@mosabua mosabua self-requested a review October 24, 2023 17:44
@mosabua
Copy link
Member

mosabua commented Oct 24, 2023

Can you also review @willmostly and @vishalya .. from my perspective this looks good.

@mosabua
Copy link
Member

mosabua commented Oct 24, 2023

Can you change commit message to these maybe

  • Correct code snippets in API docs
  • Add support to use external URL for backend creation

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/support_external_url branch from 1d6b8bf to d79762b Compare October 25, 2023 00:45
@Chaho12
Copy link
Member Author

Chaho12 commented Oct 25, 2023

Rebased and changed commit message as you said :)

@lambrospetrou
Copy link

Thanks for this fix. Can we also change the view to use the externalUrl instead of the proxyTo since when some are only resolved internally, we need to use the externalUrl to be able to inspect the queries.

See #81 for context.

@mosabua
Copy link
Member

mosabua commented Oct 31, 2023

@Chaho12 can you please resolve the conflict

@willmostly @vishalya @andythsu - can I get your review with your input

@mosabua
Copy link
Member

mosabua commented Oct 31, 2023

@lambrospetrou I think it would be good to show both in the UI. This can be done via a separate PR by @Chaho12 or yourself or whoever.

@Chaho12
Copy link
Member Author

Chaho12 commented Nov 1, 2023

Thanks for this fix. Can we also change the view to use the externalUrl instead of the proxyTo since when some are only resolved internally, we need to use the externalUrl to be able to inspect the queries.

@lambrospetrou you can have a go to fix it! Or i can do it next week or so cause am off for holiday for the rest of the week.

@Chaho12 Chaho12 force-pushed the feature/jaeho.yoo/support_external_url branch from d79762b to 74572dd Compare November 1, 2023 03:22
Copy link
Member

@andythsu andythsu left a comment

Choose a reason for hiding this comment

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

looks good to me!

@mosabua mosabua merged commit d949c73 into trinodb:main Nov 1, 2023
2 checks passed
@Chaho12 Chaho12 deleted the feature/jaeho.yoo/support_external_url branch November 1, 2023 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants