Lambda Pipeline sync deployment strategy #1452
Conversation
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
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) {
|
|
Code coverage for golang is
|
| } else { | ||
| // Update traffic to the secondary and keep it as new secondary. | ||
| if secondary, ok := trafficCfg["secondary"]; ok { | ||
| trafficCfg["secondary"] = provider.VersionTraffic{ | ||
| Version: secondary.Version, | ||
| Percent: float64(100 - percent), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
How about using else if instead of using if inside else statement?
| } else { | |
| // Update traffic to the secondary and keep it as new secondary. | |
| if secondary, ok := trafficCfg["secondary"]; ok { | |
| trafficCfg["secondary"] = provider.VersionTraffic{ | |
| Version: secondary.Version, | |
| Percent: float64(100 - percent), | |
| } | |
| } | |
| } | |
| } else if secondary, ok := trafficCfg["secondary"]; ok { | |
| // Update traffic to the secondary and keep it as new secondary. | |
| trafficCfg["secondary"] = provider.VersionTraffic{ | |
| Version: secondary.Version, | |
| Percent: float64(100 - percent), | |
| } | |
| } |
There was a problem hiding this comment.
I get your point 👍 To be honest, I quite don't like that else if statement 😓
The reason is that else if is short for
- } else if (primary.Version == version) && (secondary, ok := trafficCfg["secondary"]); ok {
+ } else if secondary, ok := trafficCfg["secondary"]; ok {The if in else if condition should at least related to the first if to avoid misunderstanding. For example, in case A v B v C = 1 then if A else if B else (if C) is fine.
There was a problem hiding this comment.
Okay I got your point exactly 👍 We have no explicit coding style guide, but have common ground here: avoid being deeply nested as much as possible.
So how about this?
// Update traffic to the secondary and keep it as new secondary.
if secondary, ok := trafficCfg["secondary"]; ok {
trafficCfg["secondary"] = provider.VersionTraffic{
Version: secondary.Version,
Percent: float64(100 - percent),
}
}
// Make the current primary version as new secondary version in case it's not the latest built version by rollout stage.
if primary.Version != version {
trafficCfg["secondary"] = provider.VersionTraffic{
Version: primary.Version,
Percent: float64(100 - percent),
}
}There was a problem hiding this comment.
In that case, the above (Update traffic to the secondary and keep it as new secondary.) is redundant when the built version is not equal to the current primary version 😂 Edit the secondary first, then reset it to another value right after is quite weird to read, I guess 🤔
There was a problem hiding this comment.
We've actually avoided being deeply nested by doing like that, but I'm unfamiliar with the standard for this case. Besides, it's a matter of taste, I respect your decision 👍
|
Looks like |
Nice catch, I think it safe to be deleted 👍 |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
/lgtm |
|
Nice. 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?: