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

Update install.md and improve dev container setup and fix some spelling mistakes. #1689

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Update install.md and improve dev container setup and fix some spelling mistakes. #1689

merged 5 commits into from
Nov 8, 2023

Conversation

neilschark
Copy link
Contributor

Hi,

as I use the dev container setup daily for my work, I made some improvements to make using this setup easier. Some of these changes are specific to my use case, but everything which is a general improvement is added in this PR.

And i fixed some spelling mistakes I found on the way.

@hellt
Copy link
Member

hellt commented Nov 1, 2023

Hi @neilschark
thanks for your contribution.
Since the contributor of the dev container part is @ankudinov I will let him comment/eval the improvements first.

docs/install.md Outdated
Comment on lines 218 to 219
&& apt-get upgrade -y
apt-get install -y --no-install-recommends \
Copy link
Member

Choose a reason for hiding this comment

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

@neilschark
I wouldn't do the upgrade here, it will take quite some time and will leave you with a different base OS more frequently then package updates.
Not sure it is worth it, but I may be wrong.

What seems to be clearly missing here though is && after upgrade

Copy link
Contributor Author

@neilschark neilschark Nov 7, 2023

Choose a reason for hiding this comment

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

I don't think time is a problem here, as the creation of the dev container from scratch depending on the added feature already takes several minutes if you have a slow Internet connection. But after the first building everything is cached, so it doesn't matter anymore, as it is not run again until you pull a clean base image.

I get you second point though, it makes the image possibly different. I don't think it matters much, as for example the docker in docker feature is added with the 'latest' tag, so you will have this problem anyway.

In my experience the base image of debian is updated less enough so that you get some updates in this step regularly. I would guess that the image provided by Microsoft which is the base here is updated even less, so updates with security fixes would likely occur.

But at the and it is not a mayor difference either way, so I would leave it up to the project (and therefore you) to decide.

Just to have it complete I just add the missing '&&' you mentioned. Simply either accept the PR like it is now or tell me to remove the update line, I am fine with it both ways.

docs/install.md Outdated Show resolved Hide resolved
neilschark and others added 3 commits November 7, 2023 23:31
Co-authored-by: Roman Dodin <dodin.roman@gmail.com>
…containerlab into neilschark-devcontainer-docs
@hellt
Copy link
Member

hellt commented Nov 8, 2023

thnx, lets ship it and later down the road we will see if that works for everybody

@hellt hellt merged commit fd8d37b into srl-labs:main Nov 8, 2023
1 check passed
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.

None yet

2 participants