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

Fix for humanize time in #5005 #5079

Merged
merged 12 commits into from
Jan 19, 2021

Conversation

gunadhya
Copy link
Contributor

Fixes: #5005
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

Added go-humanize
I couldn't figure out how to get milliseconds values in the go-humanize library so created a workaround for that.

User facing changes (remove if N/A)

Format for printing output here

Follow-up Work (remove if N/A)

@gunadhya gunadhya requested a review from a team as a code owner November 29, 2020 16:07
@google-cla google-cla bot added the cla: yes label Nov 29, 2020
@codecov
Copy link

codecov bot commented Nov 29, 2020

Codecov Report

Merging #5079 (f61028e) into master (35214eb) will increase coverage by 0.01%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5079      +/-   ##
==========================================
+ Coverage   71.82%   71.84%   +0.01%     
==========================================
  Files         387      387              
  Lines       13928    13943      +15     
==========================================
+ Hits        10004    10017      +13     
- Misses       3190     3191       +1     
- Partials      734      735       +1     
Impacted Files Coverage Δ
pkg/skaffold/diagnose/diagnose.go 15.15% <0.00%> (ø)
pkg/skaffold/util/util.go 83.70% <86.66%> (+0.37%) ⬆️
pkg/skaffold/build/cache/retrieve.go 65.62% <100.00%> (ø)
pkg/skaffold/runner/build_deploy.go 69.89% <100.00%> (ø)
pkg/skaffold/runner/deploy.go 49.29% <100.00%> (ø)
pkg/skaffold/runner/dev.go 69.50% <100.00%> (ø)
pkg/skaffold/runner/load_images.go 94.73% <100.00%> (ø)
pkg/skaffold/runner/timings.go 95.12% <100.00%> (ø)

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 35214eb...f61028e. Read the comment docs.

@tejal29
Copy link
Member

tejal29 commented Dec 3, 2020

Thanks a lot @gunadhya for your PR.

can you please show the change output and how the new line looks?

Also, can you please switch to human readable time elsewhere https://github.com/GoogleContainerTools/skaffold/search?q=time.Since%28start%29

Thanks
Tejal

Copy link
Member

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @gunadhya for your PR.

can you please show the change output and how the new line looks?

Also, can you please switch to human readable time elsewhere https://github.com/GoogleContainerTools/skaffold/search?q=time.Since%28start%29

Thanks
Tejal

@gunadhya
Copy link
Contributor Author

gunadhya commented Dec 3, 2020

Hi @tejal29, So the output would look something like this : Deployments stabilized 3 seconds ago. Here's the doc example. Should I make the changes to other files in this format too?

@tejal29
Copy link
Member

tejal29 commented Dec 4, 2020

Hi @tejal29, So the output would look something like this : Deployments stabilized 3 seconds ago. Here's the doc example.

Thanks!

Should I make the changes to other files in this format too?

yes please.

@pull-request-size pull-request-size bot added size/L and removed size/S labels Dec 7, 2020
@gunadhya
Copy link
Contributor Author

gunadhya commented Dec 7, 2020

I've made the changes as requested. Please do let me know if anything else seems broken or need fixing.

Comment on lines 283 to 286
if time.Since(start).Seconds() < 1 {
showsTime = time.Since(start).String() + " ago"
}
logrus.Infoln("Ran", showsTime)
Copy link
Member

Choose a reason for hiding this comment

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

this would make an excellent utility function so you don't have to repeat the code.

Suggested change
if time.Since(start).Seconds() < 1 {
showsTime = time.Since(start).String() + " ago"
}
logrus.Infoln("Ran", showsTime)
logrus.Infoln("Ran", util.showHumanizeTime(start))

and

def showHumanizeTime(start time.Time) string {

showsTime := humanize.Time(start)
if time.Since(start).Seconds() < 1 {
	return  time.Since(start).String() + " ago"
}
return humanize.Time(start)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a function in util and reworked its usage in the files.

@gunadhya gunadhya requested a review from tejal29 December 9, 2020 19:04
pkg/skaffold/util/util.go Outdated Show resolved Hide resolved
@gunadhya
Copy link
Contributor Author

I'm not sure why this test is failing? https://travis-ci.com/github/GoogleContainerTools/skaffold/jobs/464991267#L1013

Copy link
Contributor

@IsaacPD IsaacPD left a comment

Choose a reason for hiding this comment

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

Everything looks good, just some changes to the test should be made.

pkg/skaffold/util/util_test.go Outdated Show resolved Hide resolved
pkg/skaffold/util/util.go Outdated Show resolved Hide resolved
@IsaacPD IsaacPD added the kokoro:run runs the kokoro jobs on a PR label Jan 12, 2021
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 12, 2021
@IsaacPD IsaacPD added the kokoro:force-run forces a kokoro re-run on a PR label Jan 15, 2021
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Jan 15, 2021
@tejal29 tejal29 merged commit c62d6f5 into GoogleContainerTools:master Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

humanize time in "stabilized in 5.23494327s"
4 participants