-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
chore: Copy http2_util, logger_conf and logger_util to util project #21
chore: Copy http2_util, logger_conf and logger_util to util project #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two minor edits/comments
@ghislainbourgeois, |
What about checking whether we can also add the |
They look quite similar, I will try to make sure we can use only |
logger_conf/conf.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghislainbourgeois,
Does not logger/logger.go
already have similar content as this file? or am I overlooking something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files seem to be used in slightly different ways for different loggers. I am not comfortable moving this right now, and I think we should plan a logging cleanup as a separate task after we centralize those dependencies.
@ghislainbourgeois, is this PR ready to merge? Or are you planning to address my previous comment about the |
I have started looking in that previous comment, and evaluating if we can easily merge that code. I will let you know today what direction I choose. |
I think this is ready to merge now, and in the near future we should do a logging cleanup. |
Ok, thanks. As soon as I merge it, I am going to create a new release/tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@ghislainbourgeois, |
Copy
http2_util
,logger_conf
andlogger_util
toutil
project.Once merged and released, dependent projects will be modified to remove the dependency on the previous small projects to simplify maintenance.