Skip to content
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.

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

🌱 Fix Permission Issue for process.txt in Tiltfile Configuration #1436

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

maxrantil
Copy link

@maxrantil maxrantil commented Jan 30, 2024

What this PR does / why we need it:
This PR fixes a setup issue: start.sh: line 34: can't create process.txt: Permission denied. Modifications in the Tiltfile ensure process.txt is now correctly created with full permissions (0777).

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 30, 2024
@tuminoid
Copy link
Member

Q: non-root as in host side, or container side? For container side non-root we remove the securityContexts for Tilt, so it runs as root for Tilt development.

@mquhuy
Copy link
Member

mquhuy commented Jan 30, 2024

Could you explain the commit message a little bit more? I'm having a hard time matching the description there with what gets changed.

@smoshiur1237
Copy link
Member

I am also having difficulties to understand the commit message and the changes. Also It seems to run as root.

@maxrantil maxrantil force-pushed the tilt-setup-overhaul/max branch 2 times, most recently from 3a8e6c4 to f4a8807 Compare January 30, 2024 09:37
@maxrantil maxrantil changed the title 🌱 Improve Tiltfile with Process Management 🌱 Fix Permission Issue for process.txt in Tiltfile Configuration Jan 30, 2024
@maxrantil
Copy link
Author

I have now updated the Title, commit message and explanation. I totally understand your confusion and the lack of explanation.

@mquhuy & @smoshiur1237 thanks for pointing that out. It was a mistake from my side, an earlier commit message was hanging.

@tuminoid I'm not sure if it is container-side or host. I have notice what you mentioned about bypassing the securityContexts and that is something I will try to change next, so we are more similar to the CAPI setup. I got this error when trying to start up tilt: /start.sh: line 34: can't create process.txt: Permission denied
I then looked into CAPI and found out that this was the solution they used.

@tuminoid
Copy link
Member

@tuminoid I'm not sure if it is container-side or host. I have notice what you mentioned about bypassing the securityContexts and that is something I will try to change next, so we are more similar to the CAPI setup. I got this error when trying to start up tilt: /start.sh: line 34: can't create process.txt: Permission denied I then looked into CAPI and found out that this was the solution they used.

OK, so the issue is that the process.txt does not pre-exist, and it cannot create it, but if it does exist, then it works. Sounds really weird, since we are running as root in the container. CAPI is doing the same securityContext removal, but in just different place in their Tilt setup. That said, there is no harm adding this.

@tuminoid
Copy link
Member

/override test-centos-e2e-integration-main test-ubuntu-integration-main

@metal3-io-bot
Copy link
Contributor

@tuminoid: Overrode contexts on behalf of tuminoid: test-centos-e2e-integration-main, test-ubuntu-integration-main

In response to this:

/override test-centos-e2e-integration-main test-ubuntu-integration-main

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/test-infra repository.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2024
Copy link
Member

@smoshiur1237 smoshiur1237 left a comment

Choose a reason for hiding this comment

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

LGTM

@kashifest
Copy link
Member

Not sure if I understood what we are trying to do here but lets see if it helps.
/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2024
@metal3-io-bot metal3-io-bot merged commit 65ce281 into metal3-io:main Jan 31, 2024
16 checks passed
@metal3-io-bot metal3-io-bot deleted the tilt-setup-overhaul/max branch January 31, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
Status: Done / Closed
Development

Successfully merging this pull request may close these issues.

6 participants