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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Singleton implementation for GetHostName. #85

Merged
merged 2 commits into from Jul 21, 2021

Conversation

Tatsinnit
Copy link
Member

@Tatsinnit Tatsinnit commented Jul 20, 2021

This PR intends to Fix the multiple GetHostName calls to Singleton call using Sync.Once.Do

This PR also try to attempt and simplify the deep parameter passing mechanism. Why using Sync.Once because its a thread safe go existing pkg funct which we can use. https://pkg.go.dev/sync#Once .

Please note: feel free to add your variable naming preferences but this way will ensure that the call for the actual run command only happens once. Advantage of this approach is that all changes reside under utils umbrella, no extra func parameter will be required, and it will guarantee the actual code is ran only once.

Another, pro is that the call to the method stays same, under the hood it only gets called once. For future any new diagnostics or anything which needs to call the utils.GetHostName don't need to worry about running many times, and no more parameter passing as well.

  • Tested with Image local: docker.io/tatsat/singleton-call-changes

This pR also simplifies this PR: #82

Thanks guys. 馃檹

Screen Shot 2021-07-20 at 9 11 12 PM

Unit test result

Screen Shot 2021-07-21 at 2 06 46 AM

davidkydd
davidkydd previously approved these changes Jul 20, 2021
Copy link
Collaborator

@davidkydd davidkydd left a comment

Choose a reason for hiding this comment

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

Looks good, maybe add a unit test or two?

@Tatsinnit Tatsinnit mentioned this pull request Jul 20, 2021
@davidkydd davidkydd dismissed their stale review July 20, 2021 10:23

Forgot testability

@davidkydd
Copy link
Collaborator

Sorry for premature approval - forgot about testability, this will maintain current tight coupling which we should move away from, and would make it impossible to inject a value of hostname for unit testing those components which consume it. We should either pass the value as a parameter or abstract the singleton behind an interface.

@Tatsinnit
Copy link
Member Author

Tatsinnit commented Jul 20, 2021

Sorry for premature approval - forgot about testability, this will maintain current tight coupling which we should move away from, and would make it impossible to inject a value of hostname for unit testing those components which consume it. We should either pass the value as a parameter or abstract the singleton behind an interface.

Cool, Tight coupling as to unit testing? I doubt if this can be go-mocked. (Hence bunch of legacy codes is interesting to unit-test) I tried adding test but until we mock RunCommandOnHost we cannot mock any of this? (Feel free to give more suggestions). Current unit test utilises Gomega.

Functionally: Current Util.GetHostName is decoupled method called from anywhere to get HostName.

Working Unit Test Added and here are the Run Result

With slight refactor here is the 100% unit test coverage of the functionality, also in future once RunCommandOnHost is replaced with actual package based runCommand, we need to remove any dependency of this command which start with RunCommand... .

Screen Shot 2021-07-21 at 2 06 46 AM

@Tatsinnit Tatsinnit requested a review from davidkydd July 20, 2021 13:19
Copy link
Collaborator

@davidkydd davidkydd left a comment

Choose a reason for hiding this comment

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

Callers are still tightly coupled to this implementation but think I'm not explaining it properly, and it would be a premature optimization anyway so this is perfect for now. Thanks!

@Tatsinnit Tatsinnit merged commit 1d59ea0 into Azure:master Jul 21, 2021
@Tatsinnit Tatsinnit deleted the feature/refactor-gethostname-call branch September 2, 2021 18:30
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.

Diagnoser WriteToCrd broken during recent refactor End-to-end - CLI test.
2 participants