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

vscode devcontainer #3474

Merged
merged 11 commits into from
Aug 9, 2022
Merged

vscode devcontainer #3474

merged 11 commits into from
Aug 9, 2022

Conversation

luwol03
Copy link
Contributor

@luwol03 luwol03 commented Aug 4, 2022

I already shared this in this in #3429 (comment) . I used this setup for my latest two pr #3451 , #3473 and it works great.

@SchrodingersGat what do you think of that? What IDE are you using locally?

I will create this PR as draft and if the team likes this, I'll do some improvements I have in mind.

@SchrodingersGat
Copy link
Member

@luwol03 I might need to get my head around this.

I use vscode for dev, with InvenTree running in docker-desktop:

This means that I can shell into the running containers via vscode, it's very clean! It also means that I am always running using the development docker environment.

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 5, 2022

@SchrodingersGat I improved it even a step further, maybe you will also like the debugger and the available syntax highlighting. I recorded a little video to show the features. Note, that the first time start will take some longer time, as the requirements needs to be fetched and the docker file be build.

devcontainer.mp4

image

Copy link
Member

@matmair matmair 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 @luwol03 ; Please move the dependency update out the rest will be a great improvement.

.vscode/launch.json Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
@matmair matmair added docker Docker / docker-compose setup Relates to the InvenTree setup / installation process labels Aug 5, 2022
@matmair matmair added this to the 0.9.0 milestone Aug 5, 2022
@luwol03
Copy link
Contributor Author

luwol03 commented Aug 5, 2022

@matmair thank you for your review. Ill take a deeper look later.

Help

Good idea, please suggest useful extensions (with their identifier) we should add.

What is the identifier image

Advantages

The good thing about this is, that this also works 1:1 with GitHub codespaces if someone is using that. Its similar to gitpod, but hosted by GitHub.

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 5, 2022

@matmair can you share the identifiers of the extensions you recommend?

@matmair
Copy link
Member

matmair commented Aug 5, 2022

@luwol03 Github codespaces are actually pretty expensive compared to gitpod in my experience.

@matmair
Copy link
Member

matmair commented Aug 5, 2022

@luwol03 the changes to enable my testing setup in devcontainers turned out to be quite involved.
I think it is better to add some docs to CONTRIBUTING for this changeset and merge it before this PR gets out of hand. I will make a separate PR once I figure out how to streamline the whole thing (would currently need 5+ extensions, a lot of setup and an old node.js version)

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 5, 2022

@luwol03 Github codespaces are actually pretty expensive compared to gitpod in my experience.

Yeah, just wanted to mention that one if somebody will need it.

@luwol03 the changes to enable my testing setup in devcontainers turned out to be quite involved.
I think it is better to add some docs to CONTRIBUTING for this changeset and merge it before this PR gets out of hand. I will make a separate PR once I figure out how to streamline the whole thing (would currently need 5+ extensions, a lot of setup and an old node.js version)

So this PR can be marked as ready to review for your side and you'll add your extensions in another PR?

@matmair matmair marked this pull request as ready for review August 5, 2022 22:11
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
@SchrodingersGat
Copy link
Member

SchrodingersGat commented Aug 6, 2022

@luwol03 while I am hesitant to have "too many" ways to achieve the same thing (in this case a development setup) this does seem like a great way to get up and running very quickly.

There are some downsides I can see:

  • Limited to sqlite (fine for simple dev)
  • Doesn't run the background worker process (again, fine for simple dev)

One major upside is the simple debugging integration, I really like that!

I'd be happy to see this as a "get a quick dev environment setup" tutorial (documented of course), with a lead-in to the full development setup e.g. "for a more fully featured development setup, refer to the development docker guide" or something like that.

@matmair what do you think about this? How do you see this vscode integration fitting in with our current setup guide (we have 4 currently!)

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 6, 2022

@SchrodingersGat Thank you for your feedback.

You're not limited to sqlite. You can also configure a docker compose stack in the devcontainer.json and attach to a specified service. This would be helpful to setup a database, redis, ...

Questions

  1. How do I start the work?
  2. How do we make a select which of the featured levels you'd like to start? Because I guess for first time contributor who has no idea about the setup, the current state is just fine, but for you as core developer, you need more different things like different database, worker, redis, ...

@SchrodingersGat
Copy link
Member

I'm not sure how useful it is to fully flesh out with multiple containers etc. Even having the background worker not running is a good thing for development as it forces the processes to run in the server thread - great for debugging.

As for "which features should you include"... I don't know. Every new setup is another thing we need to document and support.

I'm happy to promote this as a "developer setup" guide, and anything beyond is left to the user.

The current setup guides do not really help with new developers who want to get stuck into coding. This would be a great addition in this regard. We should add a new setup guide to the docs specifically for developers. Your video would work well there I think

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 6, 2022

If we want to use that video, I would like to make a new one which shows more available features. Maybe we should also wait until @matmair added some more extensions?

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 6, 2022

@SchrodingersGat Please let me know what the next steps will be.

I'm currently preparing a video which we can shared in the docs.

@SchrodingersGat
Copy link
Member

@luwol03 I am currently running through the process of setting up on my system to replace my dev setup.

One thing I see is that we already have a dockerfile which you are essentially copying (the first part of at least).

Is it possible to inherit from the Dockerfile at the top level directory? That way if we ever have to update our docker file, at least we won't have to change two files?

@SchrodingersGat
Copy link
Member

SchrodingersGat commented Aug 7, 2022

I ran into this error during the initial setup process:

image

Have you seen this?

Update: Fixed in the patch file below!

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 7, 2022

Is it possible to inherit from the Dockerfile at the top level directory? That way if we ever have to update our docker file, at least we won't have to change two files?

I don't know of any way, because we need the vscode image as base. I'd be happy if you know any ways.

@luwol03 I made the following patch with some small additions for a "cleaner" initial setup.
Please apply the patch and push the changes.

Thank you for the additions. They look good. I'll apply it for you, but you can also push yourself as a maintainer to my branch. You need to checkout the refs/pulls/3348 ref I guess, but this is simpler with the gh cli, it's just gh pr checkout 3348 and then you're able to push to my branch.

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 7, 2022

@SchrodingersGat There are three things I have to notice here,
1.

   // Use 'postCreateCommand' to run commands after the container is created.
-  "postCreateCommand": "mkdir -p /workspaces/InvenTree/dev/commandhistory && python3 -m venv venv && . venv/bin/activate && cd /workspaces/InvenTree && pip install invoke && inv update && mkdir -p dev && inv setup-dev",
+  "postCreateCommand": "mkdir -p /workspaces/InvenTree/dev/commandhistory && cd /workspaces/InvenTree && python3 -m venv dev/venv && . dev/venv/bin/activate && pip install invoke && inv update && inv setup-test --ignore-update --path dev/demo-dataset",

If we do it like this, we should make sure that setup-test don't run if already data exists in the database, because this command runs every time you remove you recreate your devcontainer (e.g. rebuild, delete all exited containers and then start the devcontainer again, ...). But I prefer it as a task (Use cmd pallet type > Run task press enter, select setup-test press ok, wait until complete)

    "INVENTREE_PLUGIN_DIR": "/workspaces/InvenTree/dev/plugins",
    "INVENTREE_PLUGIN_FILE": "/workspaces/InvenTree/dev/plugins.txt",

Isnt there also a INVENTREE_PLUGINS_ENABLED needed?

 "name": "Python: Django - 3rd party",

Do you also want to change that?

- Add extra environment variables for InvenTree config
- Move venv into dev directory (cleaner structure)
- Ensure base package requirements get installed
- Handle write-permission error for compiling translations
- Install test data inside dev directory
@luwol03
Copy link
Contributor Author

luwol03 commented Aug 7, 2022

We can share this video if you want.

out.mp4

@SchrodingersGat
Copy link
Member

If we do it like this, we should make sure that setup-test don't run if already data exists in the database, because this command runs every time you remove you recreate your devcontainer (e.g. rebuild, delete all exited containers and then start the devcontainer again,

Ah good point. I guess it makes more sense to run the "demo data" setup as a separate action.

The setup-dev command failed locally for me, I'll have to look into that some more.

@SchrodingersGat
Copy link
Member

@luwol03 I'll have another look at this hopefully tomorrow :)

@SchrodingersGat SchrodingersGat added the enhancement This is an suggested enhancement or new feature label Aug 7, 2022
@luwol03
Copy link
Contributor Author

luwol03 commented Aug 7, 2022

@SchrodingersGat Can you please share the error you had with setup-dev?

@matmair
Copy link
Member

matmair commented Aug 7, 2022

Looks good. Don't wait for the extra extensions @luwol03 , I have to rewrite several tasks for it to work seamlessly.

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 7, 2022

@matmair no, I'm not waiting. I already posted the video. Currently I'm waiting for @SchrodingersGat to answer my notices from his patch in #3474 (comment) . If that's all resolved, I think we can merge this from my side.

@SchrodingersGat
Copy link
Member

Can you please share the error you had with setup-dev?

Looks like a permission error. Still see it when running the task.

image

.vscode/tasks.json Outdated Show resolved Hide resolved
.devcontainer/postCreateCommand.sh Outdated Show resolved Hide resolved
@SchrodingersGat
Copy link
Member

Getting pretty close @luwol03

Regarding the demo video, I would suggest a simplification to the workflow. There should be only three steps to get "up and running":

Download / Build

Download (clone) code and build the devcontainer

Demo Dataset

Install demo dataset with the setup-test task

Debug Server

Launch the server in debug mode

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 8, 2022

And what is with showing the needed extension?

@SchrodingersGat
Copy link
Member

And what is with showing the needed extension?

Sorry I don't understand what you are asking

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 8, 2022

@SchrodingersGat The remote containers extension is needed for all that to work. I showed that at the beginning after cloning the repo in my last video.

@SchrodingersGat
Copy link
Member

Ah understood. Yes that absolutely needs to be in there. I really liked the structure of your video. There are a number of smaller things that I guess I did not specify in my "simple three step plan" above :)

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 8, 2022

Can you describe exactly what you want to be in the video? And another idea would be to add litte texts to the video like Requirements, building, debugging, ... (the chapters)

@SchrodingersGat
Copy link
Member

@luwol03 I have opened a new issue on the docs site - inventree/inventree-docs#350 - where we should discuss the documentation aspect on this. Let's keep this PR for the actual feature :)

@SchrodingersGat
Copy link
Member

I think the only outstanding item here is to work out the setup-dev bug which was a permission issue. @luwol03 do you have any idea why it might have failed on my end?

I'm running vscode under windows 10 if that makes any difference.

@luwol03
Copy link
Contributor Author

luwol03 commented Aug 9, 2022

@SchrodingersGat Regarding to your bug, have you checked the file permissions in the container? The vscode user needs at least execute and read. Maybe find out which exact command that is and try running it directly instead through the post task.

I can only test on macos, and cannot reproduce that. I'll try that one more time later.

@SchrodingersGat
Copy link
Member

@luwol03 I'll have a look into that further. I'm happy to accept this as is, might just be my setup.

Can you have a look at inventree/inventree-docs#350 when you have some time?

@SchrodingersGat SchrodingersGat merged commit 2d63122 into inventree:master Aug 9, 2022
@luwol03
Copy link
Contributor Author

luwol03 commented Aug 9, 2022

Oh nice, that this is merged. Yes, I already had a look at the other issue on the docs site. Great team btw.

@luwol03 luwol03 deleted the feature/devcontainer branch August 9, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Docker / docker-compose enhancement This is an suggested enhancement or new feature setup Relates to the InvenTree setup / installation process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants