Skip to content

Conversation

@delta592
Copy link
Contributor

@delta592 delta592 commented Aug 9, 2025

This PR shouldn't be this crazy, but @wass3r absolutely required I fix many years of reviewdog failures before @wass3r would even considering a review and merge. @plyr4 @ecrupper i don't mind, but it ballooned my original changes, so this review might take a while to review.

My actual changes are in commit 58cc3f0

here's hoping @wass3r doesn't need codecov to review. if so, i can attempt to fix those as well.

@delta592 delta592 self-assigned this Aug 9, 2025
@codecov
Copy link

codecov bot commented Aug 9, 2025

Codecov Report

❌ Patch coverage is 90.60052% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.13%. Comparing base (14f88b0) to head (a1999de).

Files with missing lines Patch % Lines
cmd/vela-worker/exec.go 85.45% 6 Missing and 2 partials ⚠️
cmd/vela-worker/operate.go 0.00% 7 Missing ⚠️
cmd/vela-worker/run.go 0.00% 7 Missing ⚠️
cmd/vela-worker/start.go 0.00% 6 Missing ⚠️
executor/linux/secret.go 20.00% 4 Missing ⚠️
runtime/docker/container.go 86.36% 1 Missing and 2 partials ⚠️
runtime/kubernetes/container.go 83.33% 1 Missing ⚠️

❌ Your project status has failed because the head coverage (70.13%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #654       +/-   ##
===========================================
+ Coverage   58.24%   70.13%   +11.88%     
===========================================
  Files         123      123               
  Lines        8368     8517      +149     
===========================================
+ Hits         4874     5973     +1099     
+ Misses       3283     2301      -982     
- Partials      211      243       +32     
Files with missing lines Coverage Δ
cmd/vela-worker/flags.go 100.00% <ø> (+100.00%) ⬆️
executor/linux/build.go 73.60% <100.00%> (-1.91%) ⬇️
executor/linux/service.go 79.15% <100.00%> (ø)
executor/linux/step.go 78.39% <100.00%> (ø)
executor/local/build.go 73.19% <100.00%> (-2.49%) ⬇️
executor/local/service.go 93.75% <100.00%> (ø)
executor/local/step.go 89.83% <100.00%> (ø)
mock/docker/config.go 100.00% <100.00%> (+100.00%) ⬆️
mock/docker/container.go 96.23% <100.00%> (+96.23%) ⬆️
mock/docker/distribution.go 100.00% <100.00%> (+100.00%) ⬆️
... and 31 more

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

delta592 added 12 commits August 9, 2025 00:32
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
@delta592 delta592 changed the title feat: add container resource limits and build isolation tracking feat!: add container resource limits and build isolation tracking Aug 9, 2025
Signed-off-by: Dax Johnson <[email protected]>
@wass3r
Copy link
Collaborator

wass3r commented Aug 9, 2025

Apologies for the misunderstanding. I did say I wasn't going to look at these changes at almost 1am on Saturday 😅 but I never indicated why. The reviewdog/codecov checks are not required checks in our setup (although reviewdog/diff-review arguably should be), so I wouldn't ask anyone to pass them, because we would have just made those required checks in Actions.

Are there outstanding golangci-lint (reviewdog) issues? 100%. Unless we create PRs to directly address them - and we have chipped away at it over time throughout our codebases - we just ask that you take care of any that come up in your diff (hence why i mention above that maybe reviewdog/diff-review should be required).

So, sorry you went through that. Addressing all outstanding golangci-lint issues would be something to consider for a separate PR, so perhaps those changes can be salvaged yet.

For fun, I tried applying our golangci-lint rules to https://github.com/kubernetes/kubernetes and I got over 500 errors 😵 . There are definitely a bunch of relatively picky requirements, and maybe a few we want to review at some point to determine if we want to keep them around or if they're just noise; we've adjusted some over time.

We're wrapping up the current release (and might be distracted with other priorities), so it might be a few before we get to review the core changes within this PR, but thanks for taking a stab at it! 🙇🏼

Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
Signed-off-by: Dax Johnson <[email protected]>
@delta592
Copy link
Contributor Author

delta592 commented Aug 9, 2025

Thanks for the context and guidance! Now I understand more what needs to be fixed for only related items in CI.

I did open a separate PR #655 for just the code changes and those respective golangci-lint and tests. I can close this PR in favor of the other.

@delta592 delta592 closed this Aug 9, 2025
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.

3 participants