-
Notifications
You must be signed in to change notification settings - Fork 18
build: Add snapshot generator #1183
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
Changes from all commits
6f3be52
cb18f23
193ab68
c1a7e41
2a16a29
aa99279
4a8b4c7
0cc1597
3f259e3
d3b6bdb
9cded33
e3513d2
8c155a7
55d0862
f60dca1
2421584
489a906
69fbd27
961133c
122446b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,3 +51,7 @@ playwright/.cache/ | |
|
|
||
| # Ignore docs | ||
| **/*.md | ||
| **/docs/ | ||
|
|
||
| # Ignore test cache | ||
| **/.tox/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| services: | ||
| # Test server that has all the plugins loaded. Test sets are run against this server. | ||
| deephaven-plugins-docs-test-server: | ||
| container_name: deephaven-plugins | ||
| build: | ||
| dockerfile: ./Dockerfile | ||
| pull: true # We need to always pull the latest server image used in the Dockerfile | ||
| ports: | ||
| - '${DEEPHAVEN_SNAPSHOT_PORT:-10090}:10000' | ||
| environment: | ||
| - START_OPTS=-Xmx4g -DAuthHandlers=io.deephaven.auth.AnonymousAuthenticationHandler -Ddeephaven.console.type=python | ||
|
|
||
| # Extract the code blocks from the docs in the plugins directories and output them ssto a results directory. | ||
| deephaven-plugins-docs-extractor: | ||
| image: ghcr.io/deephaven/salmon-extractor | ||
|
mofojed marked this conversation as resolved.
|
||
| pull_policy: always | ||
| container_name: deephaven-plugins-docs-extractor | ||
| command: | ||
| [ | ||
| 'sh', | ||
| '-c', | ||
| 'node extract.cjs --directory /extract/plotly-express --output /results/plotly-express; node extract.cjs --directory /extract/ui --output /results/ui', | ||
| ] | ||
| volumes: | ||
| - ./plugins/ui/docs:/extract/ui | ||
| - ./plugins/plotly-express/docs:/extract/plotly-express | ||
| # exclude doc build directories by overriding with empty volumes | ||
| # We could extract from the build directories, but we need to output the snapshots to the build directory itself for Salmon to pick up... | ||
| - /extract/plotly-express/build/ | ||
| - /extract/ui/build/ | ||
| - ./docker/build/test-sets:/results # Output to a temporary build directory | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could specify this as a docker volume at the top level of the compose file instead and then it won't need an intermediary directory on the host. The potential downside is less visibility into junk in this directory if it's not cleared by the extractor before running. Docker will create and manage the volume instead of using the host as a middleman. volumes:
test-sets:
services:
extractor:
volumes:
- test-sets:/resultsYou can also still use the root volume definition to create a bind mount if you wanted it visible in the repo files. I think it works like this for the volume definition and then same to attach it to the service. The benefit of this would just be there's no accidental typo in the definition between the 2 containers, but that doesn't really matter in this case since it's only used twice volumes:
test-sets:
driver: local
driver_opts:
o: bind
device: ./docker/build/test-sets
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interestingly Copilot suggested exactly this, but with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So doing it this way throws an error if the directory doesn't already exist: Not sure how to get around that without creating the directories first (outside of the docker compose file). It sounds like the path must exist for it to work: https://forums.docker.com/t/can-named-volume-in-docker-compose-file-create-a-folder-if-it-does-not-exist/143625 I think I'll just switch this back? Unless we want to git check-in those folders then ignore the contents...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doing it which way doesn't work? The bind mount as a named volume? I'm fine without it, just thought it was worth trying. Only 2 locations and it would throw or not work if they didn't match. Or the non-bind named volume where Docker handles it and it doesn't create the intermediate test-set files in the repo. Unless you want easier inspection of the test-set files
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bind mount as a named volume doesn't work if the directory doesn't already exist. |
||
|
|
||
| # Read the test sets from the results directory and run them against the test server, taking a snapshot of the results. | ||
| deephaven-plugins-docs-snapshotter: | ||
| image: ghcr.io/deephaven/salmon-snapshotter | ||
| pull_policy: always | ||
| depends_on: | ||
| deephaven-plugins-docs-test-server: | ||
| condition: service_healthy | ||
| deephaven-plugins-docs-extractor: | ||
| condition: service_completed_successfully | ||
| environment: | ||
| HOST_URL: 'http://deephaven-plugins-docs-test-server:10000' | ||
| command: | ||
| [ | ||
| 'sh', | ||
| '-c', | ||
| 'node snapshot.cjs --directory /test-sets/plotly-express --output /results/plotly-express; node snapshot.cjs --directory /test-sets/ui --output /results/ui', | ||
| ] | ||
| volumes: | ||
| - ./docker/build/test-sets:/test-sets | ||
| # Map all the results back to the snapshots directory for those docs | ||
| - ./plugins/ui/docs/snapshots:/results/ui | ||
| - ./plugins/plotly-express/docs/snapshots:/results/plotly-express | ||
|
|
||
| # Validate MDX and the snapshots that were written are valid | ||
| deephaven-plugins-docs-validator: | ||
| image: ghcr.io/deephaven/salmon-validator | ||
| pull_policy: always | ||
| volumes: | ||
| - ./plugins/ui/docs/build/markdown:/validate/ui | ||
| - ./plugins/plotly-express/docs/build/markdown:/validate/plotly-express | ||
| - ./docker/build/validator-results:/results | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| build/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a section about running this with
npmas well? It looks like the flag just runs thenpmscript under the hood. Should we even make it a separate flag or just bake it into the--docsflag?The annoying thing is how long the docker snapshot image takes to build. Takes me like 2.5-3 min. So I could see that being an argument for not running it with the docs flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I specifically had it as a separate flag as the snapshots container takes much longer to load if you haven't already built it.
I'll add a note about updating snapshots using
npm...