feat: pause duration as string with time unit #423
Conversation
Support declaring pause duration as string with optional time unit (i.e. "s", "m", "h"). If no time unit or value is an integer, seconds are inferred. closes argoproj#292
Codecov Report
@@ Coverage Diff @@
## master #423 +/- ##
======================================
Coverage 84.5% 84.5%
======================================
Files 70 70
Lines 6667 6667
======================================
Hits 5634 5634
Misses 746 746
Partials 287 287
Continue to review full report at Codecov.
|
pkg/apis/rollouts/v1alpha1/types.go
Outdated
| // special case where no unit was specified | ||
| return int32(s) | ||
| } | ||
| return int32(p.Duration.IntVal) |
There was a problem hiding this comment.
unnecessary conversion (from unconvert)
|
Overall, I think this looks great! I like the test in types.go, but I don't like how it brings our code coverage down by 30 percent since the autogenerated code is now being included. I think we can get around this by changing our test target in the Makefile to run two go test commands. The first command only runs the tests for v1alpha without coverage, and the second command runs all the other tests with coverage |
|
I haven't used codecov.io before but it looks like they support an https://docs.codecov.io/docs/ignoring-paths # .codecov.yml
ignore:
- "pkg/apis/rollouts/v1alpha1"I did play around with modifying the test command to exclude v1alpha with the following: .PHONY: test
test: test-kustomize
go test -failfast -covermode=count -coverprofile=coverage.out -coverpkg $(shell go list ./... | grep -v github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1 | xargs | tr ' ' ',') ${TEST_TARGET}It works but the console output is not ideal (it prints the huge package list multiple times). Maybe if the covecov config does not work I could just move types_test.go to a different package? |
|
I definitely think adding ignore field to the .codedev.yaml is the ideal solution. Can you try adding it and seeing if that changes the coverage? If not, I'll open a separate PR to fix this. |
|
Looks like the codecov config worked! |
|
I'm good, thanks! |
Support declaring pause duration as string with optional time
unit (i.e. "s", "m", "h"). If no time unit or value is an integer,
seconds are inferred.
closes #292