Add dotfile repo support #7337
Add dotfile repo support #7337
Conversation
roboquat
added
release-note
do-not-merge/work-in-progress
team: IDE
team: webapp
Codecov Report
@@ Coverage Diff @@
## main #7337 +/- ##
===========================================
+ Coverage 19.04% 36.22% +17.17%
===========================================
Files 2 19 +17
Lines 168 4723 +4555
===========================================
+ Hits 32 1711 +1679
- Misses 134 2879 +2745
- Partials 2 133 +131
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
csweichel
force-pushed
the
cw/dotfiles
branch
2 times, most recently
from
d2999a9 to
2ee4e7d
Compare
|
Awesome work, thanks! |
|
Hello @csweichel. I have a suggestion, it is about using the kernel provided Apart from that, I'm eagarly waiting for the dotfiles support to arrive! |
@axonasif Thank you for the feedback. I'm not sure I understand the use-case though; instead it strikes me that that approach comes with some drawbacks which make it unfavourable imho:
|
|
I tried this PR with my slightly naive dotfiles repo which just tries to set an alias using I noticed from the .dotfiles.log that we are symlinking the Beginner Q: If I just wanted to inject aliases with a dotfile, what would be the best way to do that, without overwriting the existing .bashrc in the workspace? Do I need an install script even for something as basic as this? note: GitHub codespaces does a symlink: |
|
Random idea: on new accounts, we could check their connected git provider (all of them maybe even) and if any turn up with a dotfiles repo, we could have it be there as a default option, so that it makes it even easier to setup. |
Thank you @csweichel for sharing your thoughts on the drawbacks. Now it makes sense to me why symlinks is a better and probably the only option here, couldn't think of a few of the issues you mentioned. Also I wanted to ask, do you plan to add a subcommand in the |
|
@gtsiolis could you revise the visuals? |
Great find - I've pushed a change that should prevent this behaviour, i.e. not symlink the
Good question. We could make it the default behaviour for dotfiles repo that they overwrite files when symlinking. That would be more in-line with how install scripts likely behave. WDYT? |
That would be handy to have. It's not clear however how that would work
|
I think only informing the user about shell configuration changes is enough and that it will be effective for newly created VSCODE terminals, pre-existing terminals would not be effected unless the user explicitly reload shellrc with Edit: I think I confused myself between Edit two: Yep, I tried killing all the |
|
Looking at this now! 👀 |
There was a problem hiding this comment.
Looks great, @csweichel! 🔮
Left some minor comments below that could be addressed in the next iteration. ➿
The only non-minor issue relates to the interaction of auto-saving which could be slightly confusing or not crystal clear to the user. However, it could be ok to leave this out of the scope of this PR if we'd like to get this in soon and improve in the next iterations. ❗
Approving to unblock merging but holding in case we'd like to address any comments regarding UX below.
| BEFORE | AFTER |
|---|---|
 |
 |
/hold
| <h3 className="mt-12">Dotfiles <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3> | ||
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | ||
| <div className="mt-4"> | ||
| <h4>Repo</h4> |
There was a problem hiding this comment.
suggestion: Looks better as non-abbriviated and more descriptive, no?
| <h4>Repo</h4> | |
| <h4>Repository URL</h4> |
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | ||
| <div className="mt-4"> | ||
| <h4>Repo</h4> | ||
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" /> |
There was a problem hiding this comment.
suggestion: What do you think of adding a placeholder as help text here?
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" /> | |
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" placeholder="e.g. https://github.com/username/dotfiles" /> |
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | ||
| <div className="mt-4"> | ||
| <h4>Repo</h4> | ||
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" /> |
There was a problem hiding this comment.
question: Will this work when using a repository URL without the .git suffix?
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | ||
| <div className="mt-4"> | ||
| <h4>Repo</h4> | ||
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" /> |
There was a problem hiding this comment.
issue(non-blocking): Auto-saving the input here happens in the background but is lacking
For
For
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | ||
| <div className="mt-4"> | ||
| <h4>Repo</h4> | ||
| <input type="text" value={dotfileRepo} onChange={(e) => actuallySetDotfileRepo(e.target.value)} className="w-full" /> |
There was a problem hiding this comment.
suggestion: Instead of using the section subtitle to describe this input we could use the already existing pattern of the input help text to make it clear to the user what is expected inside the input.
| @@ -153,6 +161,12 @@ export default function Preferences() { | |||
| </div> | |||
| </SelectableCard> | |||
| </div> | |||
| <h3 className="mt-12">Dotfiles <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3> | |||
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | |||
There was a problem hiding this comment.
nitpick: So far, we've generally opted for US English across the dashboard. 🇺🇸
question: Also, what do you think of moving the input help text closer to the input instead of using the section subtitle?
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | |
| <p className="text-base text-gray-500 dark:text-gray-400">Customize workspaces using dotfiles.</p> |
suggestion: A link to learn more about how this feature works (conventions, installation, etc) to the docs (https://github.com/gitpod-io/website/issues/1407) once the docs are in place could be also nice, as @jldec mentioned in #7337 (comment).
| @@ -153,6 +161,12 @@ export default function Preferences() { | |||
| </div> | |||
| </SelectableCard> | |||
| </div> | |||
| <h3 className="mt-12">Dotfiles <PillLabel type="warn" className="font-semibold mt-2 py-0.5 px-2 self-center">Beta</PillLabel></h3> | |||
| <p className="text-base text-gray-500 dark:text-gray-400">Customise every workspace using dotfiles. Add a repo below which gets cloned and installed during workspace startup.</p> | |||
| <div className="mt-4"> | |||
There was a problem hiding this comment.
issue(non-blocking): The max width of the text input element could be limited as also @jldec mentioned in #7337 (comment). I'd suggest to go with max-w-md.
| <div className="mt-4"> | |
| <div className="mt-4 max-w-md"> |
|
LGTM label has been added. Git tree hash: 2a003b3e07decccf2f17e8bd951d9aaa0564dc09
|
Too many other folks had tried this before :) I've re-deployed the branch with a clean slate. Once that's through you should be able to try this change. |
|
@csweichel I tried authenticating and getting the same redirect URL, basically meaning that my auth popup window never gets closed automatically. Even when trying to auth via GitLab, I can't proceed. Is there any technical limitation I have never encountered or am I just unlucky? 😄 |
|
@filiptronicek yeah, it's still the same. I thought it will take some time but no 😅 |
|
/werft run with-clean-slate-deployment 👍 started the job as gitpod-build-cw-dotfiles.18 |
|
Just thought I'd chime in on this - when I took this PR for a test drive, the dashboard initially (and erroneously) reported I'd been demoted to free tier. It also appeared to wipe out all workspace data. I was neither able to get around that obstacle, nor able to even spin up a workspace 🤕... but all my data was right where I left it, once back in the main dashboard. Sorry, I know this is probably of no help.. but it felt relevant considering the auth issues you encountered. Thx 🙏 |
|
/werft run 👍 started the job as gitpod-build-cw-dotfiles.19 |
Same here |
|
@JanKoehnlein @axonasif @filiptronicek I've wiped all users from this installation. Please try again. |
|
@csweichel I was able to login now, will spin up a workspace and let you know how it goes! |
There was a problem hiding this comment.
I have just tried using my dotfiles (https://github.com/filiptronicek/dotfiles) and am getting interesting results:
every time when starting a workspace, the setup screen keeps on refreshing (e.g. white-wolverine-55vly6wb)
Update: This seems to be happening even without a dotfiles repo...
| @@ -364,6 +370,148 @@ func Run(options ...RunOption) { | |||
| wg.Wait() | |||
| } | |||
|
|
|||
| func installDotfiles(ctx context.Context, term *terminal.MuxTerminalService, cfg *Config) { | |||
There was a problem hiding this comment.
not exactly sure why terminal service is needed here
|
LGTM label has been added. Git tree hash: 7bfb85c69fa5e087d088b71b7c23874c9bd5204c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akosyakov, gtsiolis Associated issue: #5198 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 |
|
Haven't heard anything about this not working. Let's merge and iterate on main. /hold cancel |
@csweichel #7337 (review) did you see or experience this one? |
|
What file does it use to boot the dotfiles? Ie, is it like an install.sh? |
|
@Milo123459 from the RFC:
|
Description
This PR implements dotfile support, whereby a user can provide a "dotfile repo" which gets cloned during workspace startup. We implement exactly what's described in the dotfile RFC.
Related Issue(s)
Fixes #5198
How to test
https://github.com/Bash-it/bash-it.git)$HOME/.dotfilescat ~/.dotfiles.logRelease Notes
Documentation
Will add an issue once it's clear how this feature will work and if it should land.