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

feat(cli): check if dotfiles install script is executable #8588

Merged
merged 15 commits into from
Jul 21, 2023

Conversation

BRAVO68WEB
Copy link
Contributor

Problem Statement

So I was having an issue regarding the installation of dotfiles. Everything was working fine until an install script was executed. This install script was installing system dependencies and using sudo commands. Hence, I was getting permission denied.

image

Solution

I added a code snippet just before the scripts were executed.

image

PS: Sorry, I didn't create an Issue first.

@github-actions
Copy link

github-actions bot commented Jul 19, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@cdr-bot cdr-bot bot added the community Pull Requests created by the community. label Jul 19, 2023
@BRAVO68WEB
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

cdrcommunity added a commit to coder/cla that referenced this pull request Jul 19, 2023
@mafredri
Copy link
Member

Thanks for the contribution! I do wonder whether or not we should do this, though. This will introduce a local change into the repo being checked out (git tracks executable bit on files). Another solution would be to chmod +x bootstrap.sh && git add bootstrap.sh && git commit in your personal repo.

Perhaps it would be safer to check the executable bit and print a pretty error explaining how to fix it? We could also run scripts via interpreter like /bin/sh, etc, but this would also require checking potential shebang (#!/bin/other) at the top of the script. And doing that might also not work properly on Windows.

@BRAVO68WEB
Copy link
Contributor Author

Hmm.. Let me look into it

@BRAVO68WEB
Copy link
Contributor Author

BRAVO68WEB commented Jul 19, 2023

This will introduce a local change into the repo being checked out (git tracks executable bit on files). Another solution would be to chmod +x bootstrap.sh && git add bootstrap.sh && git commit in your personal repo.

Yes, you are correct in that case.

So, I found this on StackOverflow.

Shall I set a git config, so that such changes are ignored, for the dotfiles cloned repo?

Perhaps it would be safer to check the executable bit and print a pretty error explaining how to fix it?

Yes, can be done too.

I am unable to choose which would be a better solution.

@BRAVO68WEB
Copy link
Contributor Author

BRAVO68WEB commented Jul 19, 2023

Yeah on second thoughts, 2nd solution would be better. Check for exec bits, and notify user about the same. I am changing this, and pushing

@BRAVO68WEB
Copy link
Contributor Author

@mafredri Can you review it!

image

@BRAVO68WEB BRAVO68WEB changed the title feat: made dotfiles install scripts executable feat: make dotfiles install scripts executable Jul 19, 2023
@BRAVO68WEB BRAVO68WEB changed the title feat: make dotfiles install scripts executable feat: check if dotfiles install script is executable Jul 19, 2023
cli/dotfiles.go Outdated Show resolved Hide resolved
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
@BRAVO68WEB
Copy link
Contributor Author

I will also try to update docs.

@BRAVO68WEB
Copy link
Contributor Author

@mafredri I have added to docs about the same

docs/dotfiles.md Outdated Show resolved Hide resolved
Co-authored-by: Muhammad Atif Ali <matifali@live.com>
@BRAVO68WEB
Copy link
Contributor Author

Added all suggested changes.

cli/dotfiles.go Outdated Show resolved Hide resolved
Co-authored-by: Muhammad Atif Ali <matifali@live.com>
cli/dotfiles.go Outdated Show resolved Hide resolved
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I may have had some grammar nits, but all in all, excellent work! Thanks a lot for your contribution and going the extra mile! ☺️

docs/dotfiles.md Outdated Show resolved Hide resolved
docs/dotfiles.md Outdated Show resolved Hide resolved
docs/dotfiles.md Outdated Show resolved Hide resolved
docs/dotfiles.md Outdated Show resolved Hide resolved
docs/dotfiles.md Outdated Show resolved Hide resolved
docs/dotfiles.md Outdated Show resolved Hide resolved
BRAVO68WEB and others added 6 commits July 21, 2023 17:46
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
@mafredri mafredri changed the title feat: check if dotfiles install script is executable feat(cli): check if dotfiles install script is executable Jul 21, 2023
@mafredri mafredri enabled auto-merge (squash) July 21, 2023 12:27
@mafredri mafredri merged commit 65583ec into coder:main Jul 21, 2023
22 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community Pull Requests created by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants