revert "cockpit/spec: bundle cockpit plugin during rpm build step" - #4778
revert "cockpit/spec: bundle cockpit plugin during rpm build step"#4778croissanne wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
$(RPM_SPEC): $(RPM_SPEC) $(NODE_MODULES_TEST)rule introduces a self-dependency on$(RPM_SPEC), which can cause confusing or non-terminating makes; drop$(RPM_SPEC)from the prerequisites so the rule only depends on$(NODE_MODULES_TEST)(and any other real inputs). - The
awkinvocation in the$(RPM_SPEC)rule embeds$(SUB_NODE_ENV)directly into the script ('{gsub(...); $(SUB_NODE_ENV) gsub(...)}1'), which will likely produce invalid awk syntax at runtime; consider moving the env manipulation outside awk or using a clean awk script without injected shell snippets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `$(RPM_SPEC): $(RPM_SPEC) $(NODE_MODULES_TEST)` rule introduces a self-dependency on `$(RPM_SPEC)`, which can cause confusing or non-terminating makes; drop `$(RPM_SPEC)` from the prerequisites so the rule only depends on `$(NODE_MODULES_TEST)` (and any other real inputs).
- The `awk` invocation in the `$(RPM_SPEC)` rule embeds `$(SUB_NODE_ENV)` directly into the script (`'{gsub(...); $(SUB_NODE_ENV) gsub(...)}1'`), which will likely produce invalid awk syntax at runtime; consider moving the env manipulation outside awk or using a clean awk script without injected shell snippets.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4778 +/- ##
==========================================
+ Coverage 79.14% 79.23% +0.08%
==========================================
Files 265 265
Lines 7015 7015
Branches 2551 2586 +35
==========================================
+ Hits 5552 5558 +6
+ Misses 1368 1359 -9
- Partials 95 98 +3
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Ok so, let's waive it for the current release, since bundling on distro infra with 2 harmless control characters seems like the better choice. And then i'll keep this open, but in draft, and se if it's possible to push the fix through upstream before the next release. |
Uh oh!
There was an error while loading. Please reload this page.