Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce kubectl argo rollout get command #230

Merged
merged 3 commits into from
Oct 31, 2019

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Oct 28, 2019

image

image

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #230 into master will decrease coverage by 1.86%.
The diff coverage is 71.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   85.28%   83.42%   -1.87%     
==========================================
  Files          49       57       +8     
  Lines        4398     5079     +681     
==========================================
+ Hits         3751     4237     +486     
- Misses        460      620     +160     
- Partials      187      222      +35
Impacted Files Coverage Δ
utils/experiment/experiment.go 97.95% <ø> (-0.05%) ⬇️
pkg/kubectl-argo-rollouts/cmd/resume/resume.go 100% <ø> (ø) ⬆️
pkg/kubectl-argo-rollouts/info/info.go 100% <100%> (ø)
pkg/kubectl-argo-rollouts/cmd/cmd.go 100% <100%> (ø) ⬆️
rollout/experiment.go 84.55% <100%> (+0.38%) ⬆️
pkg/kubectl-argo-rollouts/options/options.go 67.21% <45.45%> (-4.79%) ⬇️
pkg/kubectl-argo-rollouts/info/pod_info.go 51% <51%> (ø)
pkg/kubectl-argo-rollouts/info/experiment_info.go 58.33% <58.33%> (ø)
pkg/kubectl-argo-rollouts/info/analysisrun_info.go 71.42% <71.42%> (ø)
...ctl-argo-rollouts/viewcontroller/viewcontroller.go 73.75% <73.75%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fb19c3...e0197ed. Read the comment docs.

@jessesuen jessesuen force-pushed the kubectl-rollout-get branch 4 times, most recently from 36b5f5a to c36b66f Compare October 30, 2019 03:06
@jessesuen jessesuen force-pushed the kubectl-rollout-get branch 3 times, most recently from 4da1bb5 to 02917da Compare October 30, 2019 10:14
spec:
containers:
- args:
- FLIP=$(($(($RANDOM%10))%2)) && exit $FLIP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL $RANDOM returns a random number!

Copy link
Member

@dthomson25 dthomson25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of untested code in the info package, but I think it is fine for now since the code is for a command-line tool. We can circle back to it for v0.7, and thankfully a large part of the untested sections looks reasonable to test by creating mocks.

pkg/kubectl-argo-rollouts/info/testdata/testdata.go Outdated Show resolved Hide resolved

func NewExperimentAnalysisRollout() *RolloutObjects {
return discoverObjects(testDir + "/experiment-analysis")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome and going to make testing significantly easier!

pkg/kubectl-argo-rollouts/options/options.go Show resolved Hide resolved
)

func (o *GetOptions) Clear() {
fmt.Fprint(o.Out, "\033[H\033[2J")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming these commands clear the terminal based on the function name, but I am not 100% sure. Can you add a comment either explaining the values or put a link to somewhere that would

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of untested code in the info package, but I think it is fine for now since the code is for a command-line tool. We can circle back to it for v0.7, and thankfully a large part of the untested sections looks reasonable to test by creating mocks.

Yes. The pod and replicaset assessment code was lifted from Argo CD, so I didn't bother writing tests for them, also because the CLI test was indirectly testing them.

pkg/kubectl-argo-rollouts/cmd/get/get.go Outdated Show resolved Hide resolved
var (
colorMapping = map[string]int{
info.IconWaiting: FgYellow,
info.IconProgressing: FgBlue,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of blue since it can be challenging to read on Black, but since we are doing Blue/Green, I almost feel like we have to

pkg/kubectl-argo-rollouts/cmd/get/get.go Show resolved Hide resolved
pkg/kubectl-argo-rollouts/cmd/get/get.go Outdated Show resolved Hide resolved
pkg/kubectl-argo-rollouts/info/pod_info.go Show resolved Hide resolved
@jessesuen jessesuen force-pushed the kubectl-rollout-get branch from 02917da to 4311461 Compare October 31, 2019 00:56
@jessesuen jessesuen force-pushed the kubectl-rollout-get branch from 4311461 to e0197ed Compare October 31, 2019 01:00
@jessesuen jessesuen merged commit 801bd42 into argoproj:master Oct 31, 2019
@jessesuen jessesuen deleted the kubectl-rollout-get branch October 31, 2019 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants