-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: add limiter service written in go #9
base: main
Are you sure you want to change the base?
Conversation
@@ -25,7 +25,7 @@ spec: | |||
ingresses: | |||
- path: / | |||
port: http | |||
hostname: api.${variables.base-hostname} | |||
hostname: api-${variables.base-hostname} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the URLs to avoid SSL errors when using this example with the AWS quickstart cluster: https://github.com/garden-io/garden-aws-quickstart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we also update DNS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to test the local version of this 👍
# Production container to be used without sync mode. | ||
kind: Build | ||
type: container | ||
name: limiter-prod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we merge this, we should add "limiter-sync" and "limiter-prod" to the ECR repository default list in https://github.com/garden-io/garden-aws-quickstart
limiter/garden.yml
Outdated
type: container | ||
name: limiter | ||
build: | ||
$if: ${command.params contains 'sync' && (command.params.sync contains 'limiter' || isEmpty(command.params.sync))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue with using this.mode
here as this
is not available in the build
option. (Opening a github issue for this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the issue above is fixed. I've pushed one commit to this PR to simplify this.
Thanks so much for adding this! I think having an example for a service with sync mode in a compiled language like Go is really useful! |
This is great! One thing though, I'm slightly sceptical of adding it to this specific project. First of all, we have a version of the vote-demo in several places and if we do add this, we should probably add it to all of them. But also, we use this in demos a lot and I don't remember once having been asked about what language we're using or whether they're dynamic. With the above in mind, I'd instead suggest making this its own example in the That what way it's easily discoverable by users and we don't involve all this extra context when pointing them towards an example of live reloading Go services. |
I remember being asked that at Kubecon all the time, that's why i thought it would be great to have it in the demos as well. |
Ok ok, gotcha. But yeah, I suppose it's fine to include this here. But shouldn't we have a dedicated example as well? Just Also, are we adding this to other vote demos? Or just here? |
I can offer to update https://github.com/garden-io/vote-demo-platform as well. The forks of it should be updated by the respective owner. I can add a dedicated example too, but I think both of these shouldn't block review of this. |
|
||
--- | ||
|
||
# The sync container only provides the tools necessary for building the library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# The sync container only provides the tools necessary for building the library | |
# The sync container only provides the necessary build tools |
The limiter service is a reverse proxy written in go that adds rate limiting functionality to make the vote process more fair. This is to highlight that code synchronization works as well with compiled languages like go as with dynamic languages like python.
3a7b3aa
to
c774bc7
Compare
Used the feature implemented in garden-io/garden#4646.
The limiter service is a reverse proxy written in go that
adds rate limiting functionality to make the vote process more fair.
This is to highlight that code synchronization works as well with
compiled languages like go as with dynamic languages like python.