-
Notifications
You must be signed in to change notification settings - Fork 19
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: support multiple routes per rollout #34
feat: support multiple routes per rollout #34
Conversation
Signed-off-by: Stepan Rakitin <[email protected]>
31faef4
to
bd280b9
Compare
@svrakitin please ping me when you will resolve all comments. Thank you for contribution! |
Signed-off-by: Stepan Rakitin <[email protected]>
24b92e0
to
3d7db80
Compare
@Philipp-Plotnikov Made some changes |
pkg/plugin/plugin.go
Outdated
gatewayAPIConfig := GatewayAPITrafficRouting{} | ||
err := json.Unmarshal(rollout.Spec.Strategy.Canary.TrafficRouting.Plugins[PluginName], &gatewayAPIConfig) | ||
gatewayAPIConfig := &GatewayAPITrafficRouting{} | ||
err := json.Unmarshal(rollout.Spec.Strategy.Canary.TrafficRouting.Plugins[PluginName], gatewayAPIConfig) |
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.
json.Unmarshal(rollout.Spec.Strategy.Canary.TrafficRouting.Plugins[PluginName], &gatewayAPIConfig)
@svrakitin Dont you think that plugins:
argoproj-labs/gatewayAPI:
httpRoutes:
- first
- second
tcpRoutes:
... Will be easier to use ? And also in so case we dont need additional structure in GatewayAPIConfig, right ? We will have seperate fields HTTPRouteList []string
TCPRouteList []string |
I don't have a preference as long as it does the job. If you feel this makes more sense I'll update. |
Then, please update as it seems easier to use and less code |
Signed-off-by: Stepan Rakitin <[email protected]>
Signed-off-by: Stepan Rakitin <[email protected]>
Done. Not sure that using separate fields makes it easier though. |
Signed-off-by: Stepan Rakitin <[email protected]>
Please update description to the PR to understand how to test this feature for users who wants to test it. I think it is very useful feature and somebody wants to test it not reading the code |
Code became cleaner and tcpRoutes and httpRoutes will be seperated and easier to find needing route in manifest |
When you will finish please update me to be sure that it is ok, but I think it was last comments. We will wait a little bit on this week to give users a chance to run it locally and test it. After I or Kostis merge this PR and ping you here with congratulations ) |
Signed-off-by: Stepan Rakitin <[email protected]>
Alright, I hope that's it. Everything else can be addressed by other PRs as pure cosmetics. Thanks. |
@svrakitin great job! |
Hi all! I've built the plugin using this branch locally (run the
The official v0.0.0-rc1 version of the plugin works without problems. go version used for local build: go1.20.14 darwin/arm64 Built artifact I am using for testing. I tried to reset the Argo Rollouts Controller deployment multiple times, delete the rollout, and reapply it again, but nothing helped. Do you maybe have any idea what could be the issue here? Thank you! |
Hi @supersyn ! It seems because some process have been already using needing resources. Im using Vscode and in development in some cases I need to kill process manually to free resources to start new version of plugin. Did you check it ? Also I created PR with the fix for setHeaderRoute feature, you can try it, it would be great. |
@Philipp-Plotnikov Recent changes in upstream introduced too many conflicts and I would appreciate help with resolving those as your team seem to have much more context and opinion on how to incorporate these changes. I will only be able to look at this again after Feb 24. |
@svrakitin We will resolve the conflicts since we introduced them. Sorry for the confusion. |
Signed-off-by: Philipp Plotnikov <[email protected]>
Signed-off-by: Philipp Plotnikov <[email protected]>
Signed-off-by: Philipp Plotnikov <[email protected]>
@svrakitin it will be great if you will test it locally to be sure it is ok |
…feature Signed-off-by: Philipp Plotnikov <[email protected]>
@kostis-codefresh it seems PR is ready. I tested locally, it works fine. It will be great if someone will test it locally too.
|
Signed-off-by: Philipp Plotnikov <[email protected]>
Signed-off-by: Philipp Plotnikov <[email protected]>
Signed-off-by: Philipp Plotnikov <[email protected]>
@svrakitin I think we will merge this PR on the following week if anyone will find bugs. As we have here a lot of features |
@svrakitin our congratulations with contribution! Great job! Thank you! |
@svrakitin Documentation is now here https://rollouts-plugin-trafficrouter-gatewayapi.readthedocs.io/en/latest/features/multiple-routes/ Any feedback welcome |
Closes #33
Introduces
httpRoutes
andtcpRoutes
fields to plugin config, which allows us to reference multipleHTTPRoute
andTCPRoute
resources.Given following routes:
We can configure the plugin to update weights for each referenced route. For example:
useHeaderRoutes field indicates we will add header routes for this specific route or no