Skip to content

fix(streamer): prevent skipPath from over-matching .git prefix - #409

Open
mohit-bhandari45 wants to merge 5 commits into
shipwright-io:mainfrom
mohit-bhandari45:fix-gitignore-skip
Open

fix(streamer): prevent skipPath from over-matching .git prefix#409
mohit-bhandari45 wants to merge 5 commits into
shipwright-io:mainfrom
mohit-bhandari45:fix-gitignore-skip

Conversation

@mohit-bhandari45

@mohit-bhandari45 mohit-bhandari45 commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Changes

This fixes a bug where skipPath in the streamer would unintentionally drop files that shared the .git prefix, such as .gitignore, .gitmodules, and .gitattributes. The matcher now checks the source-relative path and only excludes a path component named .git.

The relative-path comparison also preserves Windows UNC paths correctly.

Additionally, it normalizes cross-platform paths using filepath.ToSlash prior to string matching to safeguard against path comparison bugs when the CLI is run on Windows machines.

Related Issue

Fixes #408

Type of PR

/kind bug

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Kind label has been set
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Fixed a bug in the `shp` CLI local source streamer where files prefixed with `.git` (such as `.gitignore`, `.gitmodules`, and `.gitattributes`) were incorrectly omitted from the build context.

Copilot AI balanced review requested due to automatic review settings September 3, 2026 03:16
@pull-request-size pull-request-size Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 3, 2026
@openshift-ci openshift-ci Bot added kind/bug Categorizes issue or PR as related to a bug. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 3, 2026
Copilot AI reviewed Sep 3, 2026

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@openshift-ci
openshift-ci Bot requested review from IrvingMg and qu1queee September 3, 2026 03:16
@openshift-ci

openshift-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign saschaschwarze0 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kaizakin

kaizakin commented Sep 3, 2026

Copy link
Copy Markdown
Member

/ok-to-test

@openshift-ci openshift-ci Bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 3, 2026
@kaizakin

kaizakin commented Sep 5, 2026

Copy link
Copy Markdown
Member

@mohit-bhandari45 Thanks for your contribution

apparently there's something wrong with the release notes
can you check that with the PR template and fix it

@openshift-ci openshift-ci Bot added release-note and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 5, 2026
@SaschaSchwarze0
SaschaSchwarze0 requested a balanced review from Copilot September 5, 2026 11:33
Copilot AI reviewed Sep 5, 2026
View reviewed changes

Copilot AI 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.

🟡 Changes recommended

UNC path handling and test boundary assertions must be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

pkg/shp/streamer/tar.go:34

  • This blank line contains trailing whitespace, so the changed file is not gofmt-clean. Please remove it to keep the Go source formatter-compliant.
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread pkg/shp/streamer/tar.go Outdated
Comment thread pkg/shp/streamer/tar_test.go Outdated
@SaschaSchwarze0

SaschaSchwarze0 commented Sep 5, 2026

Copy link
Copy Markdown
Member

@mohit-bhandari45 may you check the Copilot comments whether they are of relevance. Also, make sure to sign your commits, otherwise we cannot accept them. https://github.com/shipwright-io/cli/pull/409/checks?check_run_id=100509499395

@mohit-bhandari45

mohit-bhandari45 commented Sep 5, 2026

Copy link
Copy Markdown
Author

@SaschaSchwarze0 Updates are done and commits are signed.
@kaizakin can you check one more time?

@mohit-bhandari45
fix(streamer): prevent skipPath from over-matching .git prefix
e850045 
This fixes a bug where skipPath would unintentionally drop files that happened to share the .git prefix (such as .gitignore, .gitmodules) by ensuring it matches the exact .git directory boundary. Also normalizes cross-platform paths using filepath.ToSlash to prevent Windows path bugs.

Signed-off-by: mohit-bhandari45 <mohitbhandari852@gmail.com>
@mohit-bhandari45
comment 1 resolved
85222e5 
Signed-off-by: mohit-bhandari45 <mohitbhandari852@gmail.com>
@mohit-bhandari45
comment 2 resolved
1c81106 
Signed-off-by: mohit-bhandari45 <mohitbhandari852@gmail.com>
@SaschaSchwarze0
SaschaSchwarze0 requested a balanced review from Copilot September 5, 2026 13:04
Copilot AI reviewed Sep 5, 2026
View reviewed changes

Copilot AI 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.

🟡 Changes recommended

The nested .git filtering and invalid archive member handling must be corrected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

pkg/shp/streamer/tar_test.go:42

  • The claimed Windows/UNC behavior is not exercised here. filepath.ToSlash uses the host OS separator, and the unit workflow runs only on Ubuntu (.github/workflows/unit.yaml:16), so this remains a no-op in CI and cannot catch regressions in Windows filepath.Rel or separator handling. Please add Windows-specific coverage (or extract a platform-independent matcher that can be tested with Windows-style paths).
		cleanName := filepath.ToSlash(name)
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread pkg/shp/streamer/tar.go Outdated
Comment thread pkg/shp/streamer/tar_test.go Outdated
@mohit-bhandari45
fix(streamer): skip nested git directories
1fbed78 
Signed-off-by: mohit-bhandari45 <mohitbhandari852@gmail.com>
kaizakin
kaizakin reviewed Sep 5, 2026
View reviewed changes
Comment thread pkg/shp/streamer/tar_test.go Outdated
@mohit-bhandari45
fix(streamer): normalize archive member paths
b384dac 
Signed-off-by: mohit-bhandari45 <mohitbhandari852@gmail.com>
@mohit-bhandari45

mohit-bhandari45 commented Sep 5, 2026

Copy link
Copy Markdown
Author

@kaizakin @SaschaSchwarze0 Done

kaizakin
kaizakin approved these changes Sep 5, 2026
View reviewed changes

@kaizakin kaizakin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me

Thanks for your conrtibution @mohit-bhandari45

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2026
@mohit-bhandari45

mohit-bhandari45 commented Sep 8, 2026

Copy link
Copy Markdown
Author

@kaizakin what's the update here?

@kaizakin

kaizakin commented Sep 8, 2026

Copy link
Copy Markdown
Member

LGTM from my side.
i can't merge PRs. please wait for a maintainer to take a look

@mohit-bhandari45

mohit-bhandari45 commented Sep 11, 2026

Copy link
Copy Markdown
Author

@SaschaSchwarze0 Once you get time, have a look over this.

@psrvere

psrvere commented Sep 17, 2026

Copy link
Copy Markdown

@sayan-biswas - can you please review this

psrvere added a commit to psrvere/migtools-crane-plugin-buildconfig-to-shipwright that referenced this pull request Sep 17, 2026
@psrvere @claude
[BUILD-2475] fix: W67 says what shp build upload leaves out
262ef3f 
aufi's review on PR migtools#91 said shp build upload does not send the same
files as oc start-build --from-dir. I ran one directory through both on
a ROSA cluster. oc sent everything except .git/. shp from main skipped
five things. It dropped files listed in .gitignore and symlinks pointing
outside the directory. It also dropped symlinks inside the directory,
anything whose name starts with .git, and empty directories.

W67 now names the first two, because Shipwright keeps those on purpose.
The .git prefix and inner-symlink drops are bugs with fixes in review,
shipwright-io/cli#409 and #355. Empty directories are not worth a
warning. known-limitations.md lists all five with upstream links.
ADR-0011 records the rule and the follow-ups, and architecture.md gets
rule 18. The warning text is also shorter and plainer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
psrvere added a commit to psrvere/migtools-crane-plugin-buildconfig-to-shipwright that referenced this pull request Sep 17, 2026
@psrvere @claude
[BUILD-2475] fix: W67 says what shp build upload leaves out
8d6c324 
aufi's review on PR migtools#91 said shp build upload does not send the same
files as oc start-build --from-dir. I ran one directory through both on
a ROSA cluster. oc sent everything except .git/. shp from main skipped
five things. It dropped files listed in .gitignore and symlinks pointing
outside the directory. It also dropped symlinks inside the directory,
anything whose name starts with .git, and empty directories.

W67 now names the first two, because Shipwright keeps those on purpose.
The .git prefix and inner-symlink drops are bugs with fixes in review,
shipwright-io/cli#409 and #355. Empty directories are not worth a
warning. known-limitations.md lists all five with upstream links.
ADR-0011 records the rule and the follow-ups, and architecture.md gets
rule 18. The warning text is also shorter and plainer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
psrvere added a commit to psrvere/migtools-crane-plugin-buildconfig-to-shipwright that referenced this pull request Sep 17, 2026
@psrvere @claude
[BUILD-2475] fix: W67 says what shp build upload leaves out
d59d493 
aufi's review on PR migtools#91 said shp build upload does not send the same
files as oc start-build --from-dir. I ran one directory through both on
a ROSA cluster. oc sent everything except .git/. shp from main skipped
five things. It dropped files listed in .gitignore and symlinks pointing
outside the directory. It also dropped symlinks inside the directory,
anything whose name starts with .git, and empty directories.

W67 now names the first two, because Shipwright keeps those on purpose.
The .git prefix and inner-symlink drops are bugs with fixes in review,
shipwright-io/cli#409 and #355. Empty directories are not worth a
warning. known-limitations.md lists all five with upstream links.
ADR-0011 records the rule and the follow-ups, and architecture.md gets
rule 18. The warning text is also shorter and plainer.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Prateek Singh Rathore <prateek.singh.rathore@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@IrvingMg IrvingMg Awaiting requested review from IrvingMg
@qu1queee qu1queee Awaiting requested review from qu1queee
@SaschaSchwarze0 SaschaSchwarze0 Awaiting requested review from SaschaSchwarze0
+1 more reviewer
@kaizakin kaizakin kaizakin approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@kaizakin kaizakin

Labels

kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

Issues
Status: No status

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] shp build upload incorrectly excludes real files whose names start with .git

6 participants

@mohit-bhandari45 @kaizakin @SaschaSchwarze0 @psrvere @shipwright-ci-bot