bugfix: pass annotation value to strconv.ParseBool #2894
Conversation
evalutes correct variable that holds the annotation value instead of cost `helmUpgradeForceAnnotation`
|
looks like the travis build itself had an duplicate job id? |
| } | ||
|
|
||
| for _, test := range tests { | ||
| assert.Equal(t, test.expectedVal, hasHelmUpgradeForceAnnotation(annotations(test.input)), test.name) |
There was a problem hiding this comment.
Would not be required used assert in this case. We could do something such as follows but I am ok with 👍
| assert.Equal(t, test.expectedVal, hasHelmUpgradeForceAnnotation(annotations(test.input)), test.name) | |
| got := hasHelmUpgradeForceAnnotation(annotations(test.input)) | |
| if test.expectedVal != got { | |
| t.Fatalf("For hasHelmUpgradeForceAnnotation expected the value %v and got %v", test.expectedVal, got) | |
| } |
There was a problem hiding this comment.
Is assert bad? I much prefer asserts
There was a problem hiding this comment.
I just make the comment to share, however, as I said above, I am ok 👍 with move forward here. Tests are great always and it is just a nit really.
Please, feel free to merge this one if you are also ok with @jmrodri .
Just to share, regards your question
Is assert bad? I much prefer asserts
In the past, I advocate in favour of the asserts usage as well because I just was not adapted to the Testing style. However, after while I changed my mind. According to the Golang authors they have reasons to do NOT added it deliberately in the language.
See in the GoDocs:
And then following a summary of the cons:
- asserts is an third-party lib not officially supported
- arguments from the authors of The Go Programming Language book, page 302:
- Tests can feel like they're written in a different language (RSpec/Mocha for instance)
- Errors can be cryptic "assert: 0 == 1"
- Pages of stack traces can be generated
- Tests stop executing after the first assert fails - masking patterns of failure
PS.: In SDK, we have been using the asserts mainly for the scaffolds tests.
There was a problem hiding this comment.
I prefer asserts and think that some of the opinions in the go FAQ are just that, opinions.
Unless we as a community are extremely diligent about test style using the standard testing library:
- We end up with a total hodge podge of not-so-great tests.
- The failure messages are inconsistent at best and not helpful at worst.
- Writing tests take longer and more lines of code, which means time not spent adding more features or fixing more bugs AND it is more to review, understand, and maintain.
Regardless, I don't want to start a flame war 😆. We're using github.com/stretchr/testify in many (perhaps most) of our tests already, and I think consistency is the most important thing here.
| } | ||
| } | ||
|
|
||
| func annotations(m map[string]interface{}) *unstructured.Unstructured { |
There was a problem hiding this comment.
here could be a name that clarifies better such as addMockAnnotation but I am ok with as well 👍
Co-Authored-By: Camila Macedo <cmacedo@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
/cherry-pick v0.17.x |
|
@estroz: new pull request created: #3027 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Co-Authored-By: Camila Macedo <cmacedo@redhat.com> Cherry pick of #2894
Description of the change:
helm: pass
forceto strconv.ParseBool() instead ofhelmUpgradeForceAnnotationMotivation for the change:
Fixes issue where helm.operator-sdk/upgrade-force annotation value is not being respected
Closes #2884