Skip to content

fix(lint): use repo-root-relative path in git restore call in container_configs#4301

Open
Copilot wants to merge 7 commits into
devfrom
copilot/fix-container-configs-bug
Open

fix(lint): use repo-root-relative path in git restore call in container_configs#4301
Copilot wants to merge 7 commits into
devfrom
copilot/fix-container-configs-bug

Conversation

Copy link
Copy Markdown

Copilot AI commented May 26, 2026

Completed Fixes for container_configs lint

Fixes Implemented

  • Primary fix: Replaced git-diff detection with content comparison
    • Use-case: Enable testing of patches etc. to stable releases of nf-core/tools not just dev
    • Snapshots file content before generation
    • Compares new content to snapshot after generation
    • Eliminates false "out of date" failures when configs are regenerated but not committed

I think this is the right thing to do, even if you do not need testing of older releases. It just did not come up because your container configs were always update during checkout already and modules update did not change anything.

  • Secondary fix: Removed git restore dependency
    • No longer uses git restore with relative paths
    • Writes back snapshotted content directly
    • Fixes relative wf_path bugs

Tests Added

  • test_container_configs_uncommitted_but_correct_passes: Key scenario where modules update regenerated configs without committing
  • test_container_configs_working_tree_restored_after_lint: Verifies working tree restoration
  • test_container_configs_restore_with_relative_wf_path: Verifies relative path handling

PR Status

@jpfeuffer jpfeuffer marked this pull request as ready for review May 26, 2026 14:41
@github-actions

This comment was marked as resolved.

@jpfeuffer jpfeuffer changed the base branch from main to dev May 26, 2026 14:41
@jpfeuffer
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

…configs-bug

# Conflicts:
#	nf_core/pipelines/lint/container_configs.py

Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com>
Copy link
Copy Markdown
Author

Copilot AI commented May 26, 2026

@copilot resolve the merge conflicts in this pull request

Merge conflicts resolved in 388429e. The only conflicting file was container_configs.py — the resolution keeps all log.debug calls from dev alongside the repo_root fix from this branch.

Copy link
Copy Markdown
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

please add a test to tests/pipelines/lint/test_container_configs.py to test this scenario.

Copilot AI and others added 2 commits May 26, 2026 15:08
…configs lint

The old approach used repo.index.diff(None) to detect stale configs, which
compared the working tree against HEAD. This caused false 'out of date'
failures when configs were already regenerated (e.g. by modules update) but
not yet committed.

The new approach:
1. Snapshots file content on disk before generation
2. Generates new configs
3. Compares new content to the snapshot
4. If unchanged → pass; if different → fail

This makes the lint correct regardless of the git commit state. The restore
logic now simply writes back the saved content instead of using git restore,
which also fixes the original repo-root-relative path bug.

Also adds two new tests:
- test_container_configs_uncommitted_but_correct_passes: verifies that
  uncommitted but correct configs don't produce false failures
- test_container_configs_working_tree_restored_after_lint: verifies the
  working tree is properly restored after lint

Agent-Logs-Url: https://github.com/nf-core/tools/sessions/e3bf8950-f65b-4a3d-9b94-e3a275b359eb

Co-authored-by: jpfeuffer <8102638+jpfeuffer@users.noreply.github.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.68%. Comparing base (f2f69f4) to head (e11140f).

❗ There is a different number of reports uploaded between BASE (f2f69f4) and HEAD (e11140f). Click for more details.

HEAD has 103 uploads less than BASE
Flag BASE (f2f69f4) HEAD (e11140f)
python-3.10 175 72

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@jpfeuffer jpfeuffer requested a review from mashehu May 26, 2026 15:39
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.35%. Comparing base (f2f69f4) to head (2eebf84).

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

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