Lambda rollback stage implementation #1462
Conversation
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. fix case == 1This was created by todo plugin since "TODO:" was found in 19af962 when #1462 was merged. cc: @khanhtc1202. |
|
Code coverage for golang is |
|
Code coverage for golang is |
|
I was taking a walk around the implementation for lambda cloud provider, then I found out this part: Instead it'd better be: for version, weight := range cfg.RoutingConfig.AdditionalVersionWeights {
newerVersionTraffic = percentageToPercent(aws.Float64Value(weight))Also, as you may know all around here should be replaced with I should've mentioned on previous review, but hopefully it helps you. |
Thanks for your comment, I will make a separate PR for those fixes 😁 |
| } | ||
|
|
||
| originalTrafficCfg := provider.RoutingTrafficConfig{} | ||
| if err := json.Unmarshal([]byte(originalTrafficCfgData), &originalTrafficCfg); err != nil { |
There was a problem hiding this comment.
Because we defined Encode function inside the provider package, so this Decode part should be there too.
| } | ||
|
|
||
| // Restore original traffic config from metadata store. | ||
| originalTrafficKeyName := fmt.Sprintf("%s-%s-original", fm.Spec.Name, in.Deployment.RunningCommitHash) |
There was a problem hiding this comment.
Using the commit hash for the key is a nice idea. 👍
But do we need the function name?
Because the scope of these keys is inside the deployment. So maybe ("original-traffic-%s", commitHash) is good enough.
@nakabonne PTAL 😁 |
| } | ||
|
|
||
| // Restore promoted traffic config from metadata store. | ||
| promotedTrafficKeyName := fmt.Sprintf("%s-%s-promote", fm.Spec.Name, in.Deployment.RunningCommitHash) |
There was a problem hiding this comment.
nit: "latest-promote-traffic-%s", commitHash
| promotedTrafficCfgData, ok := in.MetadataStore.Get(promotedTrafficKeyName) | ||
| // If there is no previous promoted traffic config, which mean no promote run previously so no need to do anything to rollback. | ||
| if !ok { | ||
| in.LogPersister.Info("No promoted traffic config found. No need to rollback current remote traffic config.") |
There was a problem hiding this comment.
nit: "It seems the traffic has not been changed during the deployment process. No need to rollback the traffic config."
| // we need to reset any others SECONDARY created by previous (until failed) PROMOTE stages. | ||
| case 1: | ||
| // Validate stored original traffic config, since it PRIMARY ONLY, the percent must be float64(100) | ||
| primary, ok := originalTrafficCfg["primary"] |
There was a problem hiding this comment.
nit: "primary" should be defined as a const.
There was a problem hiding this comment.
just to confirm, you mean the key "primary" for those maps, is that right 👀 ( so the "secondary" as well should be defined as a const too, I guess )
There was a problem hiding this comment.
Yes, the key of the map. 👍 (Because it is shared around the code.)
|
Code coverage for golang is
|
|
The following files are not gofmt-ed. By commenting pkg/app/piped/executor/lambda/lambda_test.go--- pkg/app/piped/executor/lambda/lambda_test.go.orig
+++ pkg/app/piped/executor/lambda/lambda_test.go
@@ -17,8 +17,9 @@
import (
"testing"
- provider "github.com/pipe-cd/pipe/pkg/app/piped/cloudprovider/lambda"
"github.com/stretchr/testify/assert"
+
+ provider "github.com/pipe-cd/pipe/pkg/app/piped/cloudprovider/lambda"
)
func TestConfigureTrafficRouting(t *testing.T) {
|
|
@nghialv @nakabonne PTAL 🙏 |
|
Code coverage for golang is
|
|
|
||
| func (c *RoutingTrafficConfig) Decode(data []byte) bool { | ||
| if err := json.Unmarshal(data, c); err != nil { | ||
| return false |
There was a problem hiding this comment.
I think it would be better to return "error" instead of "bool", example for logging.
Same with the "Encode" function.
|
Code coverage for golang is
|
|
Thank you. |
|
🚀 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: