chore(operator): eradicate Python step 1#18754
Closed
Conversation
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- The
status-checkjob referencesneeds.e2e-testbut this workflow does not define ane2e-testjob, which will cause the job to fail at runtime; either add the job or remove it fromneeds. - In the
test-go-implementationjob, theUpload Go bundle artifactsstep usesif: steps.check-go.outputs.exists == 'true'but there is nocheck-gostep defined, so this condition should be updated or the guard removed. - The Go
patch-csvcommand currently panics but is already wired intodispatch.shand used whenUSE_GO_BUNDLE_HELPER=true; consider returning a clear error instead of panicking so callers and CI get a more controlled failure mode during the migration period.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `status-check` job references `needs.e2e-test` but this workflow does not define an `e2e-test` job, which will cause the job to fail at runtime; either add the job or remove it from `needs`.
- In the `test-go-implementation` job, the `Upload Go bundle artifacts` step uses `if: steps.check-go.outputs.exists == 'true'` but there is no `check-go` step defined, so this condition should be updated or the guard removed.
- The Go `patch-csv` command currently panics but is already wired into `dispatch.sh` and used when `USE_GO_BUNDLE_HELPER=true`; consider returning a clear error instead of panicking so callers and CI get a more controlled failure mode during the migration period.
## Individual Comments
### Comment 1
<location> `operator/bundle_helpers/pkg/yamlformat/pyaml_compat.go:15` </location>
<code_context>
+// - Single quotes for simple string values
+// - Empty string as '' not ""
+// - Flow style for arrays/maps where appropriate
+func EncodePyYAMLStyle(w io.Writer, data interface{}) error {
+ var buf bytes.Buffer
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying EncodePyYAMLStyle by avoiding a no-op normalization layer and extra buffer until concrete PyYAML-style transformations are needed or clearly documenting it as a future hook.
You can reduce indirection and conceptual overhead here without changing behavior.
Right now `EncodePyYAMLStyle` + `normalizeToPyYAMLStyle` introduce:
- An extra buffer and write path
- An exported symbol that promises PyYAML-style output which is not yet implemented
- A named post-processing step that is a no‑op
Two concrete simplifications that preserve all current behavior:
1. **Remove the extra buffer and postpone normalization until it exists**
```go
// EncodePyYAMLStyle encodes data to YAML.
//
// NOTE: This currently delegates directly to yaml.Encoder with 2‑space indent.
// It is intended as the single call site to add PyYAML-style normalization
// once we know the exact rules we need to match.
func EncodePyYAMLStyle(w io.Writer, data interface{}) error {
encoder := yaml.NewEncoder(w)
encoder.SetIndent(2)
if err := encoder.Encode(data); err != nil {
return err
}
return encoder.Close()
}
```
You can reintroduce a `normalizeToPyYAMLStyle` buffer step later when you have concrete transformations to apply, keeping call sites unchanged.
2. **If you want to keep the hook now, make the placeholder explicit and internal**
```go
func EncodePyYAMLStyle(w io.Writer, data interface{}) error {
var buf bytes.Buffer
encoder := yaml.NewEncoder(&buf)
encoder.SetIndent(2)
if err := encoder.Encode(data); err != nil {
return err
}
if err := encoder.Close(); err != nil {
return err
}
output := normalizeToPyYAMLStyle(buf.Bytes())
_, err := w.Write(output)
return err
}
// normalizeToPyYAMLStyle is a placeholder; it currently returns yaml.v3 output
// unchanged. PyYAML-compatible formatting will be implemented here once
// finalized.
func normalizeToPyYAMLStyle(input []byte) []byte {
return input
}
```
Key points:
- Either clarify in the doc comment that PyYAML compatibility is *not yet* implemented and this is a future hook, or
- Avoid the abstraction until you have concrete normalization behavior.
Both options keep existing functionality but reduce confusion and mental overhead for readers.
</issue_to_address>
### Comment 2
<location> `operator/bundle_helpers/cmd/fix_descriptors.go:78` </location>
<code_context>
cmd := exec.Command(normalizerPath)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at 7a223d5. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18754 +/- ##
=======================================
Coverage 49.48% 49.48%
=======================================
Files 2661 2661
Lines 200782 200782
=======================================
+ Hits 99350 99353 +3
+ Misses 94015 94012 -3
Partials 7417 7417
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@github-actions[bot]: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@github-actions[bot]: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@github-actions[bot]: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
porridge
force-pushed
the
konflux/porridge/bundle-helpers-rewrite-fixorder
branch
from
87516b3 to
56f040c
Compare
porridge
changed the title
porridge
changed the title
GrimmiMeloni
added a commit
that referenced
this pull request
Feb 10, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
5 tasks
GrimmiMeloni
added a commit
that referenced
this pull request
Feb 16, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
GrimmiMeloni
added a commit
that referenced
this pull request
Feb 27, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
GrimmiMeloni
added a commit
that referenced
this pull request
Mar 10, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
GrimmiMeloni
added a commit
that referenced
this pull request
Mar 23, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
GrimmiMeloni
added a commit
that referenced
this pull request
Mar 24, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
GrimmiMeloni
added a commit
that referenced
this pull request
Mar 25, 2026
Port csv-patcher logic from PR #18800 into single-binary architecture from PR #18754. Implement patch-csv subcommand with version handling, image replacement, and related images logic. Go implementation is now the default (USE_GO_BUNDLE_HELPER defaults to true) with Python retained for fallback. Add local comparison script for rapid validation during development. Byte-by-byte equivalence with Python implementation verified for all related-images modes. User request: Combine PR 18754 (porridge's Go rewrite with equivalence testing) and PR 18800 (complete csv-patcher implementation). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
Superseded by #18947 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!