Skip to content
This repository has been archived by the owner on Jan 19, 2024. It is now read-only.

Several errata fixes in the description of DUNE. #84

Merged
merged 5 commits into from
Dec 28, 2022

Conversation

jolly-fellow
Copy link
Contributor

No description provided.

@jolly-fellow jolly-fellow added the documentation Improvements or additions to documentation label Dec 12, 2022
@jolly-fellow jolly-fellow self-assigned this Dec 12, 2022
@jolly-fellow
Copy link
Contributor Author

Hey @jgiszczak could you approve this PR if the text is OK.

>+ Rebuild the image by calling ./bootstrap.sh.
>+ Add a root directory of host system to list of shared directories in Docker Desktop settings.
>+ Add a root directory of the host system to the list of shared directories in Docker Desktop settings.

Choose a reason for hiding this comment

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

Shouldn't this be "Add a directory..."? In a Unix-style system there's only one root directory and it's not customarily shared with containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In DUNE it is definitely shared with containers. It is not my decision and I'd prefer to share directories in /home but it is a part of DUNEs design and it is very difficult to change on this stage. So I propose to leave it as is or make a new issue for this task and merge this PR which fix erratas.

Choose a reason for hiding this comment

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

It really does require the entire filesystem to be shared with the container?

In that case, the wording should be "Add the root directory..."

And yes that's awful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It really does require the entire filesystem to be shared with the container?

I guess this is not known exactly until the end of development. So at the beginning they decided to share all to don't limit themselves in case they will need to have access somewhere in the file system.

In that case, the wording should be "Add the root directory..."
And yes that's awful.

IMHO it will be possible to change when we will release it and get feedback from real users. We will know better what is really need and what is not. Then it will be easier to make decision about limiting accesses, rights and so on.

So now, if you have no other remarks, I will be glad if you approve this PR and I will merge it.

Choose a reason for hiding this comment

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

I guess this is not known exactly until the end of development. So at the beginning they decided to share all to don't limit themselves in case they will need to have access somewhere in the file system.

In that case, the text needs to change as I suggested. Sharing the entirety of the filesystem is so unusual that the ambiguity needs to be removed. I assumed something else was meant because it's so unusual. I even suggest adding a disclaimer sentence, incorporating your explanation in this thread into the document. I assure you I won't be the only person questioning the document if there's no explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, the text needs to change as I suggested. Sharing the entirety of the filesystem is so unusual that the ambiguity needs to be removed. I assumed something else was meant because it's so unusual. I even suggest adding a disclaimer sentence, incorporating your explanation in this thread into the document. I assure you I won't be the only person questioning the document if there's no explanation.

Let's solve problems as they come. I decided to add this PR because I was confused by architecture of Docker. I was solving of problem with files shared between the host and the container and I was sure that Docker Desktop is just a frontend over dockerd daemon. I was very surprised to found that Docker Desktop works even if the dockerd is shot down. So I decided to highlight this information for the users of DUNE to save their time.

If someone asks about why do they need to share whole root with the container I will write additional topic which explain this subject in separated PR. IMHO it is better so solve one issue in one PR and merge them separately.

README.md Outdated Show resolved Hide resolved
@jolly-fellow
Copy link
Contributor Author

@jgiszczak If you have no other remarks, Please approve this PR and I will merge it.

Copy link

@jgiszczak jgiszczak left a comment

Choose a reason for hiding this comment

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

Explanation is ambiguous. Comments will have to suffice for now.

@jolly-fellow jolly-fellow requested a review from mikelik December 28, 2022 08:07
@jolly-fellow jolly-fellow merged commit be7b02b into AntelopeIO:main Dec 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants