-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add Dockerfile for testing. #49
Add Dockerfile for testing. #49
Conversation
Can you add a comment maybe at the top, that tells how to use this Dockerfile especially mentioning the:
|
tests/Docker/Dockerfile
Outdated
USER root | ||
WORKDIR /tmp | ||
|
||
CMD ["/bin/bash"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should change this to pwsh
since... It's a PowerShell container 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it look now ?
Amended with both suggestions.
f01ec62
to
3469906
Compare
3469906
to
69d9cf9
Compare
🤔 I wonder if we should add a comment to the file for those that are new to docker to explain how to build the image and run it. (AKA: I don't know docker, so I had to go find where it was referenced in the initial issue to see how to build a docker file.) |
I was thinking the same thing actually. I was about to add building instructions for Windows/Linux and Mac into the README.md itself of that directory. I think it would be best if guys would specify where should this actually reside. Currently its in tests/Docker which technically could have a really nice README inside instead of using comments inside Dockerfile. I guess the wiki pages is always a good option. There is even a nice way to alias docker images like shown here:
|
I like the idea of adding a small section to the README. That tells you how to run the Dockerfile. Just simply:
And instead, if we can move the |
🤔 Is there any particular reason for us to have a nvim and a vim user? Would it not make more sense to just have a single user that has both configs? |
The only reason why there is both users inside is to provide full isolation of user profiles for testing. Aka nothing from nvim can affect vim testing and vice versa. Might not be exactly necessary tbh. I just find this logical decoupling useful for example if I would accidentally point my nvim config at .vim/plugged rather than other location etc. |
With the addition of snippets in #55 we might want to consider adding |
#55 is in now, @JayDoubleu if you don't mind updating this PR with the 2 additional packages |
Sure thing. Will update soon with new folder structure etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Awesome work @JayDoubleu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was discussion of adding the steps to run the docker to the ReadMe. Do we want to do that in this change, or do we want to do it in a separate change?
RUN curl -sfLo ~/.vim/autoload/plug.vim --create-dirs \ | ||
https://raw.githubusercontent.com/junegunn/vim-plug/master/plug.vim | ||
|
||
RUN echo "call plug#begin('~/.vim/plugged')\n\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we have a vim and nvim user, could we store the plugins in the same spot for both users, and then instead of duplicating this code, we could write it to a common file and copy that file to the proper spot for each user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the extensions (and coc
) expect certain things to be in certain places... But this would bring the package down. Let's defer this for now.
@corbob a separate PR is probably fine for that |
No description provided.