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 .zprofile is owned by root #775

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Conversation

joshspicer
Copy link
Member

@joshspicer joshspicer commented Dec 6, 2023

We got a report that the remoteUser's .zprofile was owned by root, which was causing subsequent tools to fail to write to that file (eg: updating PATH). Along the way noticed some interesting changes introduces in #736 that I think might be the cause, or at least appears to be an anti-pattern to me.

This change reverts a previous change. I don't think we should be sourcing a .profile at all in .zprofile (mixing bash and zsh scripts). There's perhaps some extra configuration that we need to port over to .zprofile, which should be reviewed and added in a follow up change.

context/ slack

@joshspicer joshspicer changed the title revert h… Fix .zprofile is owned by root Dec 6, 2023
@markphip
Copy link
Contributor

markphip commented Dec 6, 2023

I do not think it is actually sourcing root, it is echoing that as a line of code into the file, which then sources it at runtime. That is questionable since it is a bash file and may not run.

The issue is that the file is being created by echo so it is owned by root and unlike the earlier code in this script, the owner is not changed after.

Reverting probably makes sense until a proper .zprofile can be written. Having the test should be great for the next time this change is attempted

@joshspicer
Copy link
Member Author

Yea, agreed after some more testing.

Updated to add an empty .zprofile if there isn't one already, and assert that the file is owned by remoteUser. Make sense to you @markphip ?

@markphip
Copy link
Contributor

markphip commented Dec 6, 2023

Yes, I think that is reasonable. sourcing .profile likely needs to be revisited because it seems likely there will be scenarios that does not work but I'd imagine @samruddhikhandale tested that manually and it at least is working in the simple scenarios where the file can be handled by zsh.

The file being owned by root was the main problem introduced here

@joshspicer joshspicer marked this pull request as ready for review December 7, 2023 18:35
@joshspicer joshspicer requested a review from a team as a code owner December 7, 2023 18:35
@joshspicer joshspicer merged commit 3ea4d6b into main Dec 7, 2023
11 checks passed
@joshspicer joshspicer deleted the joshspicer/remove-zprofile-profile branch December 7, 2023 18:49
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