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 boot time not returning stat file value #1655

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

govrin
Copy link
Contributor

@govrin govrin commented May 26, 2024

When BootTimeWithContext is called while cache is disabled, the value retrieved from reading the stat file isn't used. Also, I moved the call to time.Now() closer to where the uptime stat is read, to reduce the time between them. This still isn't perfect.

add missing return statement for boot time value retrieved from stat file. Also move current time fetch to be closer to where the "time since boot file" is read
Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

This PR means "the value of useStatFile is not being used" and that seems correct. It only starts being used from the second time onward when caching is enabled. Indeed, now that you mention it... thank you!

The line for currentTime follows the principle of "defining it close to where it is used," but you might be right that it should be moved closer as there are a few steps in between.

So could you fix the golangci-lint ? After that, I will merge.

@govrin
Copy link
Contributor Author

govrin commented Jun 6, 2024

golangci-lint said something about the file not being gci-ed, but when I ran it locally it didn't show this error. I also tried to run gci with the correct settings and it didn't fix the file. Can you run the CI again?

EDIT: Nevermind, found the problem :)

@shirou
Copy link
Owner

shirou commented Jun 6, 2024

Great. Thanks a lot!

@shirou shirou merged commit bf0a7e9 into shirou:master Jun 6, 2024
23 checks passed
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.

None yet

2 participants