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 race condition when running tests for pkg/skaffold/instrumentation #5267

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

MarlonGamez
Copy link
Contributor

@MarlonGamez MarlonGamez commented Jan 21, 2021

Description
Fixes a race condition that can occur when running tests for pkg/skaffold/instrumentation. To fix the issue of two threads accessing the isOnline variable, I've moved the responsibility of checking if the user is online to a new function and am calling that in skaffold's main()

@@ -33,6 +33,7 @@ type ExitCoder interface {
}

func main() {
instrumentation.SetOnlineStatus()
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should be pinging a server when the user has disable telemetry.

Copy link
Contributor Author

@MarlonGamez MarlonGamez Jan 21, 2021

Choose a reason for hiding this comment

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

The function checks if the user has metrics collection enabled and pings/doesn't ping based off of that. This is done by checking the value of the shouldExportMetrics package variable

pkg/skaffold/instrumentation/meter.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #5267 (f88a459) into master (62f4dff) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5267      +/-   ##
==========================================
- Coverage   71.90%   71.87%   -0.04%     
==========================================
  Files         388      388              
  Lines       14063    14064       +1     
==========================================
- Hits        10112    10108       -4     
- Misses       3210     3214       +4     
- Partials      741      742       +1     
Impacted Files Coverage Δ
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
pkg/skaffold/instrumentation/meter.go 70.50% <ø> (-0.72%) ⬇️
pkg/skaffold/docker/image.go 78.13% <0.00%> (-1.40%) ⬇️

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 62f4dff...f88a459. Read the comment docs.

@MarlonGamez MarlonGamez marked this pull request as ready for review January 21, 2021 21:45
@MarlonGamez MarlonGamez requested a review from a team as a code owner January 21, 2021 21:45
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.

LGTM!

@IsaacPD IsaacPD merged commit da0d63d into GoogleContainerTools:master Jan 21, 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.

3 participants