Skip to content

Conversation

cyclimse
Copy link
Contributor

@cyclimse cyclimse commented Jun 22, 2023

Summary

What's changed?

  • Updated API framework to be compatible with the new gateway

Why do we need this?

  • To be able to use route functions via the API framework with the updated gateway

How have you tested it?

  • Integration tests + unit test were updated

Checklist

  • I have reviewed this myself
  • There is a unit test covering every change in this PR
  • I have updated the relevant documentation

Details

  • As discussed, we do not configure directly via the Kong API but run the scwgw CLI in subprocesses.

@cyclimse cyclimse force-pushed the feat/update-gateway-to-vanilla-kong branch 3 times, most recently from 2e87677 to 14b8661 Compare June 22, 2023 19:08
@cyclimse cyclimse force-pushed the feat/update-gateway-to-vanilla-kong branch from 12e1c60 to 2cc1014 Compare June 27, 2023 10:16
)
raise RuntimeError("scwgw is not installed")

self.cli = cli
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I wanted to run scwgw check here to fail fast if the gateway container were not ready, but the problem was that env vars are not properly passed to the subprocess (intended behavior to avoid security issues) and that output parsing was a bit ugly.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ok to assume the gateway has to be fully set up and working before using this framework. Having checks running in here is mixing responsibilities, so I'm happy not to include it.

from scw_serverless.config.route import GatewayRoute


class Gateway(Protocol):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be a bit anti YAGMI to use a protocol when we have a single implementation, it's only useful for the unit tess.

if method not in HTTPMethod:
raise RuntimeError(f"Route contains invalid method {method.value}")

def asdict(self) -> dict[str, Any]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only is highly dependent of the gateway, so I prefer to have it in the gateway part of the code. It's what we do the other config options.

@cyclimse cyclimse self-assigned this Jun 27, 2023
@cyclimse cyclimse marked this pull request as ready for review June 27, 2023 12:50
@cyclimse cyclimse requested review from Shillaker and redanrd June 27, 2023 13:46
Copy link
Collaborator

@Shillaker Shillaker left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM 👍

)
raise RuntimeError("scwgw is not installed")

self.cli = cli
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ok to assume the gateway has to be fully set up and working before using this framework. Having checks running in here is mixing responsibilities, so I'm happy not to include it.

@cyclimse cyclimse merged commit 51015bc into main Jun 29, 2023
@cyclimse cyclimse deleted the feat/update-gateway-to-vanilla-kong branch June 29, 2023 07:48
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