run vet and lint in CI #136
Conversation
Signed-off-by: Yingrong Zhao <[email protected]>
VinozzZ
force-pushed
the
add-staticcheck
branch
from
e396d84 to
0a0efed
Compare
VinozzZ
force-pushed
the
add-staticcheck
branch
from
0a0efed to
8c1ac4f
Compare
Codecov Report
@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 77.95% 78.30% +0.35%
==========================================
Files 12 12
Lines 1016 1014 -2
==========================================
+ Hits 792 794 +2
+ Misses 142 138 -4
Partials 82 82
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -91,12 +91,12 @@ func MergeMap(target, override map[string]interface{}) map[string]interface{} { | |||
|
|
|||
| // SecretsConfig is the plugin stanza for secrets. | |||
| type SecretsConfig struct { | |||
| PluginConfig `json:",squash" yaml:",inline" mapstructure:",squash"` | |||
| PluginConfig `json:",inline" yaml:",inline" mapstructure:",squash"` | |||
There was a problem hiding this comment.
This is always confusing! 😅
I am pretty sure that the json tag does not support squash or inline. Embedded fields are always inlined and do not require a json entry at all.
I'm pretty sure that the json tag just ignores the ,inline anyway but it may be more clear (so that people aren't tricked into thinking it is doing anything) if we remove that json tag entirely.
| PluginConfig `json:",inline" yaml:",inline" mapstructure:",squash"` | |
| PluginConfig `yaml:",inline" mapstructure:",squash"` # json is inline automatically because it's an embedded field |
There was a problem hiding this comment.
I think you are right on the embedded fields are always inlined and do not require a json entry when used with encoding/json, the standard library.
However, it looks like the controller-gen requires it to be specified. I found the same `json:,inline; in their doc as well. https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html
There was a problem hiding this comment.
Huh I wonder what they are doing that it's used? 🤔
Thanks for checking, I'll try to remember this the next time I'm tempted to remove it!
| for _, condition := range job.Status.Conditions { | ||
| switch condition.Type { | ||
| case batchv1.JobComplete: | ||
| action.Status.Phase = porterv1.PhaseSucceeded | ||
| setCondition(log, action, porterv1.ConditionComplete, "JobCompleted") | ||
| break | ||
| break conditions |
There was a problem hiding this comment.
I'm unclear on the intent of the change and what we want the code to be doing. The way I'm reading this, when a condition on the job is complete/failed, we will call setCondition, and then repeat the entire for loop over again, re-evaluating the original condition that called the goto.
Is there a way we can do this without the goto statement (break to a label)? At the very least we need a lot more comments explaining what's intended and make sure we have a test to validate that this can't cause an infinite loop.
There was a problem hiding this comment.
I misunderstood the original code. I think the break statement is redundant and I just removed them
| tmp, err := ioutil.TempDir("", "porter-docs-test") | ||
| require.NoError(t, err) | ||
| defer os.RemoveAll(tmp) | ||
| tmp := t.TempDir() |
Signed-off-by: Yingrong Zhao <[email protected]>
This PR adds vet and lint stage in github action.
For linting, the PR adds staticcheck into the repo
related to: #131