Skip to content

Fix local source build #340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 27, 2025
Merged

Fix local source build #340

merged 2 commits into from
Aug 27, 2025

Conversation

@sayan-biswas
Copy link
Contributor

@sayan-biswas sayan-biswas commented Aug 26, 2025 •
edited by SaschaSchwarze0
Loading

Changes

  • Data stream was only being initialized when the source is not OCI artifact. Hence, in cases where the source itself is not specified in the Build object, the struct was left uninitialized
  • Explicitly set UID and GID to 0 in "tar" header while writing archive, to prevent permission issue with local file having different UID and GID

Fixes #332
Fixes #338

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • 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

Fixes bugs with builds from local source code

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 26, 2025
@openshift-ci openshift-ci bot requested review from qu1queee and rxinui August 26, 2025 12:30
rxinui
rxinui suggested changes Aug 26, 2025
View reviewed changes
Copy link
Contributor

@rxinui rxinui left a comment

Choose a reason for hiding this comment

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

Little comments regarding the changes

pkg/shp/streamer/util.go Outdated
Comment on lines 31 to 35

// Add explicit UID and GID
header.Uid = 0
header.Gid = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

As uid and gid 0 refers to superuser on Linux system, I'd rather set it to a different value.

Perhaps, -1 to set to the current user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right, 0 is probably incorrect.
I tried with -1 and it's syntactically incorrect.

Took a different approach here. Instead of setting the UID and GID while creating the tar, used tar command's flag --no-same-owner and --no-same-permission to ignore ownership and permission while extracting the tar through kubectl exec.

pkg/shp/cmd/build/upload.go Outdated
Comment on lines 123 to 125
// Stream can be initialized irrespective of the Build source type
u.dataStreamer = streamer.NewStreamer(restConfig, clientset)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move down to line 126 and remove the comment // Stream can be initialized irrespective of the Build source type as it does not help the codebase but ease the current PR review...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@sayan-biswas sayan-biswas force-pushed the fix-local-source-build branch from 1c4984e to 12a9a38 Compare August 27, 2025 00:03
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 27, 2025
@sayan-biswas sayan-biswas force-pushed the fix-local-source-build branch from 12a9a38 to 927c52b Compare August 27, 2025 00:10
@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 27, 2025
@sayan-biswas
Fix nil data streamer issue
5fb39f9 
Fixes shipwright-io#332

Changes:
- Data stream was only being initialized when the source is not OCI artifact. Hence, in cases where the source itself is not specified in the Build object, the struct was left uninitialized.

Signed-off-by: Sayan Biswas <sayan-biswas@live.com>
@sayan-biswas sayan-biswas force-pushed the fix-local-source-build branch from 927c52b to 7fc3831 Compare August 27, 2025 00:14
@sayan-biswas
Fix UID and GID permission issue with Local Source Build
cb0322a 
Fixes shipwright-io#338

Changes:
- Use flags "--no-same-permissions" and "--no-same-owner with "tar" command to prevent permission issue with local file having different UID and GID

Signed-off-by: Sayan Biswas <sayan-biswas@live.com>
@sayan-biswas sayan-biswas force-pushed the fix-local-source-build branch from 7fc3831 to cb0322a Compare August 27, 2025 00:21
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 27, 2025
rxinui
rxinui reviewed Aug 27, 2025
View reviewed changes
Copy link
Contributor

@rxinui rxinui left a comment

Choose a reason for hiding this comment

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

Would like a 2nd review on this, but to me it is now /lgtm

CC: @qu1queee

@rxinui
Copy link
Contributor

rxinui commented Aug 27, 2025

Also, the release note linter check is failing @sayan-biswas

@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.17.0 milestone Aug 27, 2025
@SaschaSchwarze0 SaschaSchwarze0 added the kind/bug Categorizes issue or PR as related to a bug. label Aug 27, 2025
SaschaSchwarze0
SaschaSchwarze0 approved these changes Aug 27, 2025
View reviewed changes
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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

The pull request process is described here

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2025
@SaschaSchwarze0 SaschaSchwarze0 requested a review from rxinui August 27, 2025 09:17
@rxinui
Copy link
Contributor

rxinui commented Aug 27, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit ebf16af into shipwright-io:main Aug 27, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SaschaSchwarze0 SaschaSchwarze0 SaschaSchwarze0 approved these changes

@qu1queee qu1queee Awaiting requested review from qu1queee

@rxinui rxinui Awaiting requested review from rxinui

Assignees

@rxinui rxinui

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

release-v0.17.0

3 participants

@sayan-biswas @rxinui @SaschaSchwarze0