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

Reusing global Configuration() object in k8s client models instances #211

Closed
wants to merge 2 commits into from
Closed

Reusing global Configuration() object in k8s client models instances #211

wants to merge 2 commits into from

Conversation

cuza
Copy link
Member

@cuza cuza commented Jan 4, 2024 •
edited
Loading

Reusing global Configuration() object in k8s client models instances to avoid calling setLevel which will do a potentially expensive cache clear

nemacysts
nemacysts approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

it'd be nice to UPPER_CASE the config variable :)

task_processing/plugins/kubernetes/kubernetes_pod_executor.py Outdated Show resolved Hide resolved
@cuza cuza requested a review from nemacysts January 4, 2024 23:47
nemacysts
nemacysts approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

@cuza think it's still worth trying to merge this? i'm a tad worried that folks will forget to add this config so we'd probably want to pair this with a pre-commit hook or something :/

@nemacysts
Copy link
Member

closing this for now since we went with patching the k8s clientlib instead

@nemacysts nemacysts closed this May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemacysts nemacysts nemacysts approved these changes

@jfongatyelp jfongatyelp Awaiting requested review from jfongatyelp

@EmanElsaban EmanElsaban Awaiting requested review from EmanElsaban

@KaspariK KaspariK Awaiting requested review from KaspariK

@EvanKrall EvanKrall Awaiting requested review from EvanKrall

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
@cuza @nemacysts