Fix the bug that Piped was ignoring the specified value for all Bool fields that have true as its default value #2789
Conversation
…fields that has true as its default value
|
The following files are not gofmt-ed. By commenting pkg/config/deployment_terraform_test.go--- pkg/config/deployment_terraform_test.go.orig
+++ pkg/config/deployment_terraform_test.go
@@ -18,9 +18,10 @@
"testing"
"time"
- "github.com/pipe-cd/pipe/pkg/model"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
+ "github.com/pipe-cd/pipe/pkg/model"
)
func TestTerraformDeploymentConfig(t *testing.T) {
pkg/config/deployment_kubernetes_test.go--- pkg/config/deployment_kubernetes_test.go.orig
+++ pkg/config/deployment_kubernetes_test.go
@@ -18,9 +18,10 @@
"testing"
"time"
- "github.com/pipe-cd/pipe/pkg/model"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
+ "github.com/pipe-cd/pipe/pkg/model"
)
func TestKubernetesDeploymentConfig(t *testing.T) {
|
|
Code coverage for golang is
|
|
Then just |
| "//pkg/filematcher:go_default_library", | ||
| "//pkg/git:go_default_library", | ||
| "//pkg/model:go_default_library", | ||
| "//pkg/version:go_default_library", |
There was a problem hiding this comment.
nits, not sure why does this related to this change
There was a problem hiding this comment.
No. A new file was added and I ran make gazelle. It seems that the version package is no longer needed for that trigger package.
|
/golinter fmt |
|
Code coverage for golang is
|
pkg/config/bool.go
Outdated
| func (b *TrueByDefaultBool) Value() bool { | ||
| if b == nil { | ||
| return true | ||
| } | ||
| return bool(*b) | ||
| } |
There was a problem hiding this comment.
I understand your thought about adding this new alias to explicitly mention the defaults value for this type, but since we add a new type alias, it may add unnecessary complexity to the codebase, and we have a leak in case there is no value set in the yaml file, if caller side use bool(*x) instead of this x.Value() the process will be panicked. How about using *bool type with a default value is true (using defaults package) as other places?
This is the sample test for that method: https://play.golang.org/p/5X2nruNhDZN
There was a problem hiding this comment.
Nice catch!
To be honest, I tried that way but it didn't work. And I thought it was a bug from the defaults library.
But after checking again, it seems that problem was already resolved and included in the new version.
So let me upgrade that lib to the latest version and try it again.
https://github.com/creasty/defaults/releases
nghialv
force-pushed
the
config-default
branch
from
ff260cb to
a08b70d
Compare
|
The following files are not gofmt-ed. By commenting pkg/config/deployment_terraform_test.go--- pkg/config/deployment_terraform_test.go.orig
+++ pkg/config/deployment_terraform_test.go
@@ -18,9 +18,10 @@
"testing"
"time"
- "github.com/pipe-cd/pipe/pkg/model"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
+ "github.com/pipe-cd/pipe/pkg/model"
)
func TestTerraformDeploymentConfig(t *testing.T) {
pkg/config/deployment_kubernetes_test.go--- pkg/config/deployment_kubernetes_test.go.orig
+++ pkg/config/deployment_kubernetes_test.go
@@ -18,9 +18,10 @@
"testing"
"time"
- "github.com/pipe-cd/pipe/pkg/model"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
+
+ "github.com/pipe-cd/pipe/pkg/model"
)
func TestKubernetesDeploymentConfig(t *testing.T) {
|
|
@khanhtc1202 Thanks for your nice catch! |
|
Code coverage for golang is
|
|
Nice work! It concerned me a lot, but it's no more 👍 |
|
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 #2787
Does this PR introduce a user-facing change?: