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

docker: introduce devcontainers #2282

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pavelbezpravel
Copy link
Contributor

@pavelbezpravel pavelbezpravel commented Jun 5, 2024

introduce devcontainers as a yet another way to build project

it allows you to build project with docker containers in a convenient way with IDE or cli

@xemul
Copy link
Contributor

xemul commented Jun 5, 2024

The title of this PR doesn't match its content. It's in fact two PRs -- one that introduces devcontainers, and another one that cleans existing scripts and dockerfile. Please, split

@@ -0,0 +1,10 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Some intro for those who never used devcontainers in the patch comment would be helpful

Copy link
Member

Choose a reason for hiding this comment

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

And why it's good for us.

Copy link
Member

Choose a reason for hiding this comment

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

And how to build and use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added instructions and hopefully this patch will help developers simplify their development process

COPY install-dependencies.sh /tmp/

RUN apt-get update && apt-get install -y \
RUN --mount=type=bind,source=./install-dependencies.sh,target=./install-dependencies.sh \
Copy link
Contributor

Choose a reason for hiding this comment

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

The patch comment lacks motivation of this change

Copy link

Choose a reason for hiding this comment

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

I thought it reduces layers - since we do not copy the file over to the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it reduces layers - since we do not copy the file over to the container.

yep, it's right

&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*

CMD ["/bin/bash"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This change is missing in (already laconic enough) patch comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found it with hadolint. it helps to pass OS signals to executable in a correct way (https://github.com/hadolint/hadolint/wiki/DL3025#rationale)

I've commented about it in a new one PR

it allows you to use a container as a full-featured development environment.
you can open project with IDE (clion, vscode, visual studio) inside a container in a convenient way: https://containers.dev/supporting
.devcontainer/devcontainer.json contains metadata and settings for devcontainer configuration: https://containers.dev/implementors/json_reference/

Signed-off-by: pavelbezpravel <[email protected]>
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.

4 participants