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

Automatically set execute bit when running dotfiles install script #541

Merged
merged 7 commits into from
Jul 3, 2023

Conversation

joshspicer
Copy link
Member

The bootstrap script will add the execute bit to the 'installCommand' when cloning dotfiles, if it is not already set as executable. This reduces friction in setting up dotfiles.

codspace/test-dotfiles@0a444c1 removes the execute bit from the existing test.

ref: https://github.com/github/codespaces/issues/13870

@joshspicer joshspicer requested a review from a team as a code owner June 1, 2023 16:33
src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

Looks good, added to the previous conversation.

src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
chrmarti
chrmarti previously approved these changes Jun 2, 2023
@joshspicer joshspicer force-pushed the joshspicer/dotfiles-execute-bit branch 3 times, most recently from 217d1a3 to e8aa19b Compare June 5, 2023 19:11
src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
src/test/cli.up.test.ts Outdated Show resolved Hide resolved
src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

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

More missing quotes, we missed some previously, so probably not critical, but would be good to fix now that we are looking into it.

src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
src/spec-common/dotfiles.ts Outdated Show resolved Hide resolved
src/spec-common/dotfiles.ts Show resolved Hide resolved
@joshspicer joshspicer force-pushed the joshspicer/dotfiles-execute-bit branch from 1e607ba to 46aba6b Compare June 26, 2023 22:08
@joshspicer
Copy link
Member Author

joshspicer commented Jun 26, 2023

Using both single quotes or double quotes to quote targetPath during the git clone ${targetPath} commands don't work well with shell characters (~) used in the tests. I tried manually making the directory as well, but that doesn't work well either (still unhappy when quoting)

image

@chrmarti
Copy link
Contributor

Using both single quotes or double quotes to quote targetPath during the git clone ${targetPath} commands don't work well with shell characters (~) used in the tests. I tried manually making the directory as well, but that doesn't work well either (still unhappy when quoting the

Ack, I wasn't aware of that. Let's keep it unquoted then for now, not supporting spaces in the path as we are doing now. (One option to support both might be to check for a leading ~ and put it outside quotes. Or escape spaces with a backslash.)

Maybe still make quoting of installCommand consistent (see my unresolved comments).

@joshspicer
Copy link
Member Author

Yes, made the installCommand quoting changes (didn't realize I hadn't pushed that).

@joshspicer joshspicer requested a review from chrmarti June 30, 2023 16:52
@joshspicer joshspicer merged commit c263250 into main Jul 3, 2023
@joshspicer joshspicer deleted the joshspicer/dotfiles-execute-bit branch July 3, 2023 18:20
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