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

Add devcontainer environment for vscode #4674

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Oct 11, 2022

Problem: it is hard to develop flux-core locally.

Solution: This addition will allow VSCode users to quickly run (and work in) a development container, defined under .devcontainer in the root, which contains a devcontainer.json file and a Dockerfile that uses the fluxrm/testenv:focal base to quickly pull and
spin up the container. We need the Dockerfile to support a post start command to fix a git security issue, and also add developer tools like formatters, linters, etc. This is a vanilla install, meaning it just will allow for build of the core flux, and as other developers try this out we can easily add other extensions that make the development experience better!

The one sticky bit was with git permissions - if you sign you can't commit from inside the container (no keys) and then when you go outside the container, having commit inside the container, you get a permissions issue (that can be fixed with a chown). Likely the best solution is just not to write to git (commit) from inside the container for now, and people can play around and see if there is a better way to go about it. I don't personally mind bringing up another terminal.

My thinking is that we can get a vanilla version in first, and then those that try it with VSCode that have preferences for extensions can propose adding them separately.

Signed-off-by: vsoch vsoch@users.noreply.github.com

Problem: it is hard to develop flux-core locally.
This addition will allow VSCode users to quickly
run (and work in) a development container, defined
under .devcontainer in the root, which contains a
devcontainer.json file and a Dockerfile that uses
the fluxrm/testenv:focal base to quickly pull and
spin up the container. We need the Dockerfile to
support a post start command to fix a git security
issue, and also add developer tools like formatters,
linters, etc. This is a vanilla install, meaning it
just will allow for build of the core flux, and as
other developers try this out we can easily add
other extensions that make the development experience
better!

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

This seems good to me. I'm not a VScode user so can't really comment on implementation.

The new vscode doc in the README is pretty vscode+developer specific, so I thought maybe it should be in a collapsible section, but then again there isn't much in there so it doesn't really matter at this point.

@vsoch
Copy link
Member Author

vsoch commented Oct 11, 2022

@grondo we probably need a strategy for having explicitly developer documentation - I think for Python (my pre-commit PR) it worked well to put under docs/python, but for this I wasn't sure where it would best go, because most of the content here is maaaaaaan pages! 🦇 👨

@grondo
Copy link
Contributor

grondo commented Oct 11, 2022

Yeah, works for me! My only suggestion was maybe to put in a collapsible field, but it certainly isn't critical at this point. Feel free to set MWP unless someone else has objections? (I'm not sure if there are any other developers that work on flux-core and use vscode to test this out)

@vsoch
Copy link
Member Author

vsoch commented Oct 11, 2022

@trws I thought you mentioned you might?

I would argue to not put it in a collapsible section because:

  1. It will remind us we need a strategy for developer docs (ideas?)
  2. People that could be looking for it won't easily find it
  3. It looks super up-to-date and cool to be using them!
  4. Probably most people don't scroll that far down in the README anyway... 😆

And do you mean like this?

Hello this is details!
echo raining meatballs!

I guess I've never thought to use that in a readme - didn't think it would work! Definitely need to try now!

@grondo
Copy link
Contributor

grondo commented Oct 11, 2022

I only know about collapsible sections because we already use it in the README. Like I said I'm fine without it.

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #4674 (9a8e3cc) into master (86fee84) will decrease coverage by 0.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4674      +/-   ##
==========================================
- Coverage   84.25%   83.38%   -0.87%     
==========================================
  Files         410      413       +3     
  Lines       61367    69423    +8056     
==========================================
+ Hits        51706    57891    +6185     
- Misses       9661    11532    +1871     
Impacted Files Coverage Δ
src/cmd/top/ucache.c 86.66% <0.00%> (-13.34%) ⬇️
src/modules/job-info/allow.c 76.66% <0.00%> (-12.70%) ⬇️
src/common/libzmqutil/monitor.c 80.00% <0.00%> (-11.03%) ⬇️
src/shell/builtins.c 91.66% <0.00%> (-8.34%) ⬇️
src/shell/kill.c 92.30% <0.00%> (-7.70%) ⬇️
src/common/libutil/timestamp.c 92.85% <0.00%> (-7.15%) ⬇️
src/modules/resource/acquire.c 65.04% <0.00%> (-7.05%) ⬇️
src/cmd/builtin/dmesg.c 87.58% <0.00%> (-6.86%) ⬇️
src/common/libutil/uri.c 90.47% <0.00%> (-6.83%) ⬇️
src/modules/job-manager/getattr.c 58.18% <0.00%> (-6.82%) ⬇️
... and 329 more

@vsoch
Copy link
Member Author

vsoch commented Oct 11, 2022

I only know about collapsible sections because we already use it in the README. Like I said I'm fine without it.

I am probably good either way - if someone else wants to chime in for a third vote I'll be happy to just do that!

@trws
Copy link
Member

trws commented Oct 11, 2022

I do sometimes, and can test. I’m good with MWP honestly if you are, then I can test and come back with some config files to get automatic intellisense setup for c and python. It’s a little fragile, and very slow, because of autotools but it works pretty well.

@mergify mergify bot merged commit e7d26c6 into flux-framework:master Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants