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

Setup DDEV homeadditions in entrypoint.sh #10

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

Biont
Copy link
Contributor

@Biont Biont commented Jul 28, 2023

Since playwright lives in a separate container, it does not benefit from all the homeadditions-magic that DDEV pours into the web-container.
This means that if you have some private NPM registries configured (and use private modules for tests), they are not automatically picked up by ddev playwright-install, resulting in errors during installation.

Luckily, we have access to /mnt/ddev_config, which still lets us access it.

This PR copies the existing homeadditions folder into /home/pwuser so that they are readable by the pwuser that installs playwright

@julienloizelet
Copy link
Owner

Hi @Biont,

Thanks for the PR.

Is there any reason not to use a volume for such a purpose?

Something like the following in a docker-compose.override.yaml is not working ? :

services:
  playwright:
    volumes:
      - /some/path/on/your/host/to/.npmrc:/home/pwuser/.npmrc

@Biont
Copy link
Contributor Author

Biont commented Jul 31, 2023

Yes I think this is something I should explore first. Frankly, I now wonder why this didn't occur to me before I considered the entrypoint script.

@Biont
Copy link
Contributor Author

Biont commented Aug 1, 2023

@julienloizelet I thought about this some more.

I do think there's value in making the container behave similar to the main DDEV web container.
Also, we already have the files present within the container, so why shouldn't we just use them to faciliate this kind of setup. Requiring an external mount introduces some maintenance effort for every project that uses this addon - for something that could be working out of the box (with files that already exist, just in an inconvenient place)

DDEV itself actually just grabs the entire folder and copies it into the local home directory
Would this be something we could just mimick here? It would allow even more tools to "just work"

@julienloizelet
Copy link
Owner

@Biont ,

I am already using a docker-compose.override.yaml to override the PLAYWRIGHT_TEST_DIR variable.

services:
  playwright:
    environment:
      - PLAYWRIGHT_TEST_DIR=your/playwright/directory/path

That's why I was suggesting to do the same to handle project specificity.

But why not, do you mean we could copy the entire .homeadditions folder in /home/pwuser ?

@rfay
Copy link
Contributor

rfay commented Aug 1, 2023

Just a note that docker-compose.override.yaml naming doesn't have any value that I know of. Just docker-compose.<somegoodname>.yaml is all you need.

@Biont Biont changed the title Symlink .npmrc if present in DDEV homeadditions Setup DDEV homeadditions in entrypoint.sh Aug 1, 2023
@Biont
Copy link
Contributor Author

Biont commented Aug 1, 2023

@julienloizelet I updated the PR so it sets up the entire homeadditions folder.

@rfay thanks for stopping by. While I have your attention, do you see any downsides to this approach?

@rfay
Copy link
Contributor

rfay commented Aug 1, 2023

If you're asking whether you can copy ~/.ddev/homeadditions into a separate container, I know of no reason that should be a problem; it will be best if the container is Debian/Ubuntu.

@julienloizelet julienloizelet merged commit 9f259e5 into julienloizelet:main Aug 2, 2023
@julienloizelet
Copy link
Owner

Merged.

I guess I will add something in the README (maybe just to explain how to use a .npmrc file) before publishing a release.

Thanks again.

@Biont Biont deleted the feat/setup-npmrc branch August 2, 2023 08:15
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.

3 participants