Skip to content

fix: use full shallow clone in process_file_updates#853

Open
elenagerman wants to merge 1 commit into
konflux-ci:mainfrom
elenagerman:release-1988-fix
Open

fix: use full shallow clone in process_file_updates#853
elenagerman wants to merge 1 commit into
konflux-ci:mainfrom
elenagerman:release-1988-fix

Conversation

@elenagerman

@elenagerman elenagerman commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Describe your changes

The python port from #775 used sparse shallow clone, but the bash task does a full shallow clone.
e2e file updates failed because the target file wasn’t present after sparse checkout (yq → exit 1).

This change makes prepare_repository() use a full shallow clone to match bash. git.clone() still supports sparse shallow clone when sparse_dirs is provided for other callers.

Relevant Jira

RELEASE-1988

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Summary by Qodo

Fix seed \n escape expansion in process_file_updates
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

Description

• Expand literal backslash escapes (e.g., "\\n") when writing seeded files.
• Prevent e2e seed content from producing invalid YAML after the Python port.
• Add regression test covering echo -e style escape expansion.
Diagram

graph TD
  A[/"Catalog/Tekton fileUpdates"/] --> B(["seed_target_file"]) --> C(["_expand_seed_content"]) --> D[("Target file")]
  D --> E[["git add (index_add_commit)"]]

  subgraph Legend
    direction LR
    _svc[/"Task/Job"/] ~~~ _fn(["Function"]) ~~~ _fs[("File")] ~~~ _git[["Git op"]]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Only expand newlines (and maybe tabs)
  • ➕ Avoids surprising/unsafe decoding of other unicode escapes (e.g., \uXXXX).
  • ➕ Matches the reported failure mode (literal \n causing invalid YAML).
  • ➖ Less flexible than echo -e semantics if other escapes are intentionally used.
  • ➖ May require extending the whitelist later as new needs arise.
2. Treat seeds as structured YAML/JSON instead of escape-decoding strings
  • ➕ Avoids ad-hoc escaping rules; preserves exact structure and types.
  • ➕ Reduces ambiguity around quoting and whitespace handling.
  • ➖ Larger change: requires defining/validating the seed format and updating producers.
  • ➖ More invasive migration for existing pipelines that emit string seeds.
3. Use `codecs.decode(seed, "unicode_escape")` with stricter validation
  • ➕ More explicit intent than encode/decode; can layer validation around accepted escapes.
  • ➕ Allows rejecting unknown escapes rather than silently transforming.
  • ➖ Still carries the same core risk as unicode_escape unless restricted.
  • ➖ Slightly more code than the current helper.

Recommendation: The current approach is an effective minimal fix and is well-covered by a regression test. If seed values can come from less-trusted sources or have unexpected escape sequences, consider tightening the behavior to a small whitelist (at least \\n) to avoid unintended transformations from unicode_escape (e.g., \\uXXXX, \\xNN).

Files changed (2) +35 / -1

Bug fix (1) +6 / -0
process_file_updates.pyDecode backslash escapes before writing seeded files +6/-0

Decode backslash escapes before writing seeded files

• Introduces '_expand_seed_content()' to expand backslash escapes using Python's 'unicode_escape' decoding. Applies this expansion in 'seed_target_file()' before writing the seed content to disk, ensuring literal '\\n' becomes a real newline.

scripts/python/tasks/internal/process_file_updates.py

Tests (1) +29 / -1
test_process_file_updates.pyAdd regression test for echo -e style seed escape expansion +29/-1

Add regression test for echo -e style seed escape expansion

• Updates the test index comment and adds 'test_seed_target_file_expands_echo_e_escapes()'. The new test verifies that a JSON-derived seed containing literal '\\n' is written with real newlines and a trailing newline.

scripts/python/tasks/internal/test_process_file_updates.py

@qodo-app-for-konflux-ci

This comment was marked as resolved.

Comment thread scripts/python/tasks/internal/process_file_updates.py Outdated
@elenagerman elenagerman force-pushed the release-1988-fix branch 4 times, most recently from 6446f02 to 2ab805e Compare June 25, 2026 03:58
@codecov-commenter

codecov-commenter commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.67%. Comparing base (86975a4) to head (3998a23).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #853      +/-   ##
==========================================
+ Coverage   95.65%   95.67%   +0.02%     
==========================================
  Files          72       72              
  Lines        7118     7105      -13     
==========================================
- Hits         6809     6798      -11     
+ Misses        309      307       -2     
Flag Coverage Δ
unit-tests 95.67% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
scripts/python/helpers/vcs/git.py 93.08% <100.00%> (+1.20%) ⬆️
...ipts/python/tasks/internal/process_file_updates.py 98.59% <100.00%> (-0.05%) ⬇️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86975a4...3998a23. Read the comment docs.

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

theflockers
theflockers previously approved these changes Jun 25, 2026
Comment thread scripts/python/tasks/internal/process_file_updates.py
@elenagerman elenagerman changed the title fix: expand seed \n escapes in process_file_updates fix: use full shallow clone in process_file_updates Jun 25, 2026
@elenagerman elenagerman requested a review from mmalina June 25, 2026 12:00
@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@elenagerman

Copy link
Copy Markdown
Contributor Author

/agentic_review

@elenagerman

Copy link
Copy Markdown
Contributor Author

/review

@elenagerman

Copy link
Copy Markdown
Contributor Author

/improve

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

@ach912 ach912 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lgtm

@ach912

ach912 commented Jul 1, 2026

Copy link
Copy Markdown

Production Approval Record

Field Value
Action APPROVED
Reviewer @ach912
Timestamp 2026-07-01T07:23:52.664Z

Approved

@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

use full shallow clone in process_file_updates

Signed-off-by: Elena German <elgerman@redhat.com>
Assisted-by: Cursor
@elenagerman

Copy link
Copy Markdown
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants