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 README.md #13

Closed
wants to merge 2 commits into from
Closed

Update README.md #13

wants to merge 2 commits into from

Conversation

Hoxolotl
Copy link
Contributor

updated the shell script with:
--volume "${PWD}/assets:/var/www/html/assets"
and
--publish-all \

updated the shell script with:
    --volume "${PWD}/assets:/var/www/html/assets" \
and 
    --publish-all   \
@Potherca
Copy link
Member

Uhm... which shell script?

@Potherca
Copy link
Member

Potherca commented Mar 20, 2024

Ah, I think I found it! You mean in the README?

https://github.com/SimplyEdit/simplycode-docker/blob/main/README.md?plain=1#L31-L38

docker run \
    --env "USER_GID=$(id -g)" \
    --env "USER_ID=$(id -u)" \
    --interactive \
    --rm \
    --tty \
    --volume "${PWD}:/var/www/www/api/data" \
    ghcr.io/simplyedit/simplycode-docker:main

@Hoxolotl
Copy link
Contributor Author

Yes I meant in the readme :)

@Potherca
Copy link
Member

The --publish-all is already mentioned... It might not be desirable to use that always, TBH.

As for the --volume "${PWD}/assets:/var/www/html/assets" what is it that is being mounted? And/Or what is missing now?

@Potherca
Copy link
Member

I think I tracked this back to the bash "launcher" for docker in https://github.com/SimplyEdit/simplycode/.

I'll ask @ylebre regarding the assets.

Also, I did a cleanup: SimplyEdit/simplycode#31. If so desired, we could add a check here to make sure both script and command in the README continue to be the same? (Might be overkill, not sure...)

@Hoxolotl
Copy link
Contributor Author

Ask Ylebre, he was the one who gave me that over slack, I just copy-paste-used it.

@Potherca
Copy link
Member

The reason that assets has been added, is for those cases where the developer has assets (JS, CSS, images file, etc.) they want to use in their app. It is currently considered a short-term solution/hack until a thorough, long-term solution has been decided upon.

So for now, best to leave it out.

@Potherca
Copy link
Member

The two questions are :

  1. Should we add --publish-all to the default example?
    I am inclined to say "no" as I would rather not teach a bad practice to people who do not know what it means. As it is already mentioned for the specific use-case I am inclined to leave this as-as.
  2. Should we add --volume "${PWD}/assets:/var/www/html/assets" to the deafult example?
    Here, I also think "no" BUT it might be worth adding some prose explaining that this can be done (and why, i.e. which use-case it solves for now).

- Remove extraneous whitespace
- Sort parameters alphabetically
- Remove trailing slash `/`
@Potherca
Copy link
Member

Potherca commented Jul 5, 2024

Closing for now.

@Potherca Potherca closed this Jul 5, 2024
@Potherca Potherca deleted the shellScriptUpdate branch August 27, 2024 15:36
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.

2 participants