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.

Sign up for GitHub

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

Jump to bottom

Migrate the use of fsnotify to fswatcher in cert_watcher.go #4232

Merged
merged 2 commits into from
Feb 12, 2023
Merged

Migrate the use of fsnotify to fswatcher in cert_watcher.go #4232

merged 2 commits into from
Feb 12, 2023

Conversation

haanhvu
Copy link
Contributor

@haanhvu haanhvu commented Feb 12, 2023

Signed-off-by: haanhvu haanh6594@gmail.com

Which problem is this PR solving?

First step for #3924 as discussed here

@haanhvu
Migrate the use of fsnotify to fswatcher in cert_watcher.go
24c5977 
Signed-off-by: haanhvu <haanh6594@gmail.com>
@haanhvu haanhvu requested a review from a team as a code owner February 12, 2023 17:37
@haanhvu
Copy link
Contributor Author

haanhvu commented Feb 12, 2023

@yurishkuro You said here that we need to make both static_handler.go and cert_watcher.go use fswatcher. But I already see static_handler.go use it. So I just make cert_watcher.go use it in this PR.

Please help review!

yurishkuro
yurishkuro previously approved these changes Feb 12, 2023
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

👍

@yurishkuro yurishkuro enabled auto-merge (squash) February 12, 2023 18:02
@codecov
Copy link

codecov bot commented Feb 12, 2023 •
edited
Loading

Codecov Report

Base: 97.10% // Head: 97.10% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (31577cd) compared to base (fedeb4c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #4232   +/-   ##
=======================================
  Coverage   97.10%   97.10%           
=======================================
  Files         302      302           
  Lines       17683    17685    +2     
=======================================
+ Hits        17171    17173    +2     
  Misses        412      412           
  Partials      100      100
Flag Coverage Δ
unittests 97.10% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/config/tlscfg/cert_watcher.go 94.81% <100.00%> (ø)
pkg/fswatcher/fs_watcher.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

yurishkuro
yurishkuro reviewed Feb 12, 2023
View reviewed changes
pkg/fswatcher/fs_watcher.go
@@ -34,6 +35,11 @@ func (f *fsnotifyWatcherWrapper) Add(name string) error {
return f.fsnotifyWatcher.Add(name)
}

// Close closes the watcher.
func (f *fsnotifyWatcherWrapper) Close() error {
Copy link
Member

Choose a reason for hiding this comment

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

please add a test for this func

Copy link
Contributor Author

@haanhvu haanhvu Feb 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I'll do it.

Is there any command that checks Codecov failure locally? I ran make test and as I can remember it didn't report the failure.

Copy link
Member

Choose a reason for hiding this comment

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

Not to my knowledge. You can only compare package level coverage before and after the change with go test -cover.

@haanhvu
Add test for Close() in fswatcher
31577cd 
Signed-off-by: haanhvu <haanh6594@gmail.com>
auto-merge was automatically disabled February 12, 2023 19:16

Head branch was pushed to by a user without write access

@yurishkuro yurishkuro enabled auto-merge (squash) February 12, 2023 20:20
@yurishkuro yurishkuro merged commit e751dbf into jaegertracing:main Feb 12, 2023
@yurishkuro
Copy link
Member

🎉 Thanks!

shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this pull request Feb 22, 2023
@haanhvu @shubbham1215
Migrate the use of fsnotify to fswatcher in cert_watcher.go (jaegertr…
7dc7ba6 
…acing#4232)

Signed-off-by: haanhvu <haanh6594@gmail.com>

## Which problem is this PR solving?
First step for jaegertracing#3924 as discussed
[here](jaegertracing#3924 (comment))

---------

Signed-off-by: haanhvu <haanh6594@gmail.com>
shubbham1215 pushed a commit to shubbham1215/jaeger that referenced this pull request Mar 5, 2023
@haanhvu @shubbham1215
Migrate the use of fsnotify to fswatcher in cert_watcher.go (jaegertr…
cbb30f9 
…acing#4232)

Signed-off-by: haanhvu <haanh6594@gmail.com>

## Which problem is this PR solving?
First step for jaegertracing#3924 as discussed
[here](jaegertracing#3924 (comment))

---------

Signed-off-by: haanhvu <haanh6594@gmail.com>
Signed-off-by: shubbham1215 <sawaikershubham@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@albertteoh albertteoh Awaiting requested review from albertteoh albertteoh is a code owner automatically assigned from jaegertracing/jaeger-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@haanhvu @yurishkuro