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

fix(context): Removing unneeded global context #649

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(context): Removing unneeded global context #649

wants to merge 1 commit into from

Conversation

dark0dave
Copy link
Collaborator

@dark0dave dark0dave commented Oct 25, 2024 •
edited
Loading

Please do not merge this until its ready.

@dark0dave dark0dave self-assigned this Oct 25, 2024
@dark0dave dark0dave marked this pull request as draft October 25, 2024 12:39
@migueldelucasdoit
Copy link
Collaborator

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
Copy link
Collaborator Author

dark0dave commented Oct 25, 2024 •
edited
Loading

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
Copy link
Collaborator Author

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

@dark0dave dark0dave marked this pull request as ready for review October 25, 2024 13:34
@dark0dave dark0dave changed the title draft: chore(context): Please keep the packages not depedant on each … chore(context): Removing unneeded global context Oct 25, 2024
@dark0dave
Copy link
Collaborator Author

dark0dave commented Oct 25, 2024 •
edited
Loading

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 dark0dave mentioned this pull request Oct 25, 2024
Merged
@dark0dave
Copy link
Collaborator Author

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

@dark0dave dark0dave force-pushed the fix/removeContext branch 2 times, most recently from 3668865 to 816f04d Compare October 25, 2024 14:25
@femrtnz
Copy link
Collaborator

femrtnz commented Oct 25, 2024

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

@dark0dave
Copy link
Collaborator Author

dark0dave commented Oct 25, 2024 •
edited
Loading

@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
Copy link
Collaborator Author

Maintianing this repo is not a race.

@dark0dave dark0dave mentioned this pull request Oct 25, 2024
Open
@dark0dave dark0dave linked an issue Oct 25, 2024 that may be closed by this pull request
Add mise #651
Open
@dark0dave
Copy link
Collaborator Author

dark0dave commented Oct 31, 2024 •
edited
Loading

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:

  • Place all configuration in Config
  • Where possible do not introduce intra-package bindings, this increases complexity and makes testing more difficult
  • Try not alter the entire repo, just for one flag

Please try to keep this in mind when adding changes.

@dark0dave dark0dave changed the title chore(context): Removing unneeded global context fix(context): Removing unneeded global context Oct 31, 2024
@dark0dave dark0dave added fix Fix for an issue not-stale labels Oct 31, 2024
femrtnz
femrtnz approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@femrtnz femrtnz left a comment

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@femrtnz femrtnz femrtnz approved these changes

@migueldelucasdoit migueldelucasdoit Awaiting requested review from migueldelucasdoit

At least 2 approving reviews are required to merge this pull request.

Assignees

@dark0dave dark0dave

Labels
fix Fix for an issue not-stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add mise
3 participants
@dark0dave @migueldelucasdoit @femrtnz