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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: context usage in k8s code #2405

Merged
merged 93 commits into from May 15, 2024
Merged

Conversation

lucasrod16
Copy link
Member

@lucasrod16 lucasrod16 commented Mar 26, 2024

Description

refactor context usage in k8s code

Related Issue

Relates to #2365

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

Copy link

netlify bot commented Mar 26, 2024

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit fc58ef2
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/6644b350baf89a000861893b

@lucasrod16 lucasrod16 self-assigned this Mar 26, 2024
@lucasrod16 lucasrod16 added openssf Issues needing to be completed before donation to OpenSSF tech-debt 💳 Debt that the team has charged and needs to repay labels Mar 26, 2024
src/cmd/dev.go Outdated Show resolved Hide resolved
src/cmd/tools/helm/repo_add.go Outdated Show resolved Hide resolved
src/extensions/bigbang/test/bigbang_test.go Show resolved Hide resolved
src/pkg/cluster/injector.go Show resolved Hide resolved
src/pkg/packager/common_test.go Outdated Show resolved Hide resolved
@phillebaba
Copy link
Collaborator

@lucasrod16 I had a deeper look at the k8s package and am now wondering why we even have it. Ignoring some helper functions, it for the most part just wraps a single call to the client set. In a sense it just increases complexity. My suggestion is to remove it and just use the client set directly.

@lucasrod16
Copy link
Member Author

@lucasrod16 I had a deeper look at the k8s package and am now wondering why we even have it. Ignoring some helper functions, it for the most part just wraps a single call to the client set. In a sense it just increases complexity. My suggestion is to remove it and just use the client set directly.

Removing the k8s package could be something we discuss as a team, but it is out of scope for this PR. This PR is more strictly focused on how context flows through the program to the client set.

Copy link
Collaborator

@phillebaba phillebaba left a comment

Choose a reason for hiding this comment

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

I think this looks fine. I still have some thoughts about using timer.Reset but we can revisit that at a later PR. It is a lot more valuable to get this merge to unlock a lot of other fixes.

@lucasrod16 lucasrod16 merged commit 53465d7 into main May 15, 2024
24 checks passed
@lucasrod16 lucasrod16 deleted the refactor-context-usage-in-k8s-code branch May 15, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change 💔 openssf Issues needing to be completed before donation to OpenSSF ready-for-review tech-debt 💳 Debt that the team has charged and needs to repay
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants