search

doitintl / kube-no-trouble

Easily check your clusters for use of deprecated APIs
MIT License
3.21k stars 158 forks source link

fix(context): Removing unneeded global context #649

Closed dark0dave closed 3 weeks ago

dark0dave commented 1 month ago

Please do not merge this until its ready.

migueldelucasdoit commented 1 month ago

Hello @dark0dave we would like to find the best way to collaborate together. Can you please explain in more detail what you're trying to do with this PR? Happy to discuss this with you in person and with the rest of the team.

dark0dave commented 1 month ago

Hello @dark0dave we would like to find the best way to collaborate together. Can you please explain in more detail what you're trying to do with this PR? Happy to discuss this with you in person and with the rest of the team.

This pr is to remove the context varrible which was placed in without "collaborating" with me....

dark0dave commented 1 month ago

Standards in this project have been either ignored or just gone unnoticed.

dark0dave commented 1 month ago

I'll state it for brevity again. Do not introduce unneeded global dependancies...

These packages are supposed to only lightly depend on one annother.

dark0dave commented 1 month ago

This still isn't ready please don't merge, I need to correct the tests.

femrtnz commented 1 month ago

@dark0dave please let us know when this is ready to review, thanks!

dark0dave commented 1 month ago

@femrtnz @migueldelucasdoit this pr is now ready, thank you for your patience. In future if we could avoid introducing global context varriables without discussing it that would be great.

image

dark0dave commented 1 month ago

Maintianing this repo is not a race.

dark0dave commented 1 month ago

The repeated code is required for tests. We can clean that up later. Can I get a review from @migueldelucasdoit @femrtnz.

The intent here is as I have said in line with removing global context. In an ideal situtation we do not share a global context. So instead I have introduced cusomizable printer options as I am sure many more printing requests will arise.

This leaves room for the future, rather than placing in a global context, which created a tight binding accross the sub packages. I know this is the natural inclination of golang to add a context, but we want to keep these packages seperate. Your welcome to introduce a context for indivual packages, see pkg/judge/rego.go for a nice example of how this is done.

We try to use the config package to hold configuration for the whole of kubent, so by introducing a context you effectively stored configuration in two places which is not desirable. Futher that change added context everywhere throughout the application limiting refactoring.

Main take aways:

Please try to keep this in mind when adding changes.

dark0dave commented 3 weeks ago

Hey @dark0dave thanks for detailed explanation. I agree it is a better approach removing the global context.

Whats is your plan to fix the tests? Looking the duplications I can see benefits on having those duplicated there to improve readability. Maybe we could ignore duplications on test files in Sonar? Or create a some helper functions?

Lets solve the duplicated test code in another pr, this one is too large already. I have created an issue to track it. Open to suggestions!

Sonar is not very helpful currently, so I tend to ignore it.

I agree with you the duplications currently improve readability so in my book its fine.

dark0dave commented 3 weeks ago

https://github.com/doitintl/kube-no-trouble/issues/652