-
Notifications
You must be signed in to change notification settings - Fork 83
feat(deployment): Support configuration of external third-party services (resolves #1680). #1681
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
base: main
Are you sure you want to change the base?
Changes from all commits
fb5546e
80c9de7
f6be19a
c6d5051
e3bb275
cc2aa72
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 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||
| # yaml-language-server: $schema=../usr/share/config-schemas/clp-config.schema.json | ||||||||||||||
| # | ||||||||||||||
|
Contributor
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.
Suggested change
|
||||||||||||||
| #package: | ||||||||||||||
| # storage_engine: "clp-s" | ||||||||||||||
| # query_engine: "clp-s" | ||||||||||||||
|
|
@@ -15,6 +16,8 @@ | |||||||||||||
| ## File containing credentials for services | ||||||||||||||
| #credentials_file_path: "etc/credentials.yaml" | ||||||||||||||
| # | ||||||||||||||
| ## Remove any bundled services below if you wish to use your own. For more information, see | ||||||||||||||
| ## https://docs.yscope.com/clp/main/user-docs/guides-external-database.html#configuring-clp-to-use-external-databases | ||||||||||||||
| #bundled: ["database", "queue", "redis", "results_cache"] | ||||||||||||||
|
Contributor
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. to keep consistent with the doc
Suggested change
|
||||||||||||||
| # | ||||||||||||||
| #database: | ||||||||||||||
|
|
||||||||||||||
|
Contributor
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. might need to run lint on this file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,28 +173,47 @@ When using AWS DocumentDB or MongoDB Atlas: | |
|
|
||
| ## Configuring CLP to use external databases | ||
|
|
||
| After setting up your external databases, configure CLP to use them by editing `etc/clp-config.yaml`: | ||
|
|
||
| ```yaml | ||
| database: | ||
| host: "<mariadb-hostname-or-ip>" | ||
| port: 3306 | ||
| name: "clp-db" | ||
| # Credentials will be set in etc/credentials.yaml | ||
|
|
||
| results_cache: | ||
| host: "<mongodb-hostname-or-ip>" | ||
| port: 27017 | ||
| name: "clp-query-results" | ||
| ``` | ||
| After setting up your external databases, configure CLP to use them: | ||
|
|
||
| Set the credentials in `etc/credentials.yaml`: | ||
| 1. Edit `etc/clp-config.yaml` to specify which services are bundled (managed by the `clp-package` | ||
| Docker Compose project): | ||
|
|
||
| ```yaml | ||
| database: | ||
| username: "clp-user" | ||
| password: "<your-mariadb-password>" | ||
| ``` | ||
| ```yaml | ||
| # Remove "database" and "results_cache" from this list to use external instances | ||
| bundled: | ||
| # - "database" | ||
| - "queue" | ||
| - "redis" | ||
| # - "results_cache" | ||
| ``` | ||
|
|
||
| 2. Configure the connection details for your external databases in `etc/clp-config.yaml`: | ||
|
|
||
| ```yaml | ||
| database: | ||
| host: "<mariadb-hostname-or-ip>" | ||
| port: <mariadb-port> | ||
|
|
||
| results_cache: | ||
| host: "<mongodb-hostname-or-ip>" | ||
| port: <mongodb-port> | ||
| ``` | ||
|
|
||
| :::{important} | ||
| If your external service is running on the host machine, **do not use `127.0.0.1`** as the host. | ||
| The CLP components run in containers, so `127.0.0.1` would refer to the container itself. | ||
| Instead, use: | ||
| - `host.docker.internal` to refer to the host machine. | ||
| - The host's actual network IP address (e.g., `192.168.1.10`) | ||
| ::: | ||
|
Comment on lines
+202
to
+208
Contributor
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. Host networking guidance needs platform-specific clarification. The note about Suggested addition (after line 207): - `host.docker.internal` to refer to the host machine.
- The host's actual network IP address (e.g., `192.168.1.10`)
+
+ ::::{note}
+ **For Linux Docker hosts**: `host.docker.internal` is not available. Use the actual host IP address or hostname instead.
+ ::::
Contributor
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. not sure whether this coderabbit comment is valid or not
Contributor
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. 🧩 Analysis chain🌐 Web query: 💡 Result: Short answer: Yes — but with caveats.
References
If you want, I can show the exact docker run or docker‑compose lines to use for your setup. @hoophalab The comment is valid. On Docker Desktop (Mac/Windows/Docker Desktop for Linux), For Linux Docker Engine users, they need to use Suggested documentation improvement:Update the important block in the documentation (around line 203-207) to include platform-specific guidance: :::{important}
If your external service is running on the host machine, **do not use `127.0.0.1`** as the host.
The CLP components run in containers, so `127.0.0.1` would refer to the container itself.
Instead, use:
- `host.docker.internal` to refer to the host machine (available by default on Docker Desktop)
- The host's actual network IP address (e.g., `192.168.1.10`)
**For Linux Docker Engine users** (not Docker Desktop): `host.docker.internal` requires manual
configuration. Add the following to your `docker-compose-all.yaml` services that need to
connect to the host:
```yaml
extra_hosts:
- "host.docker.internal:host-gateway"Alternatively, use the host's actual IP address instead. Learnt from: junhaoliao
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. i think this requires more thoughts / investigations. Let's put this review on hold
Contributor
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.
|
||
|
|
||
| 3. Set the credentials in `etc/credentials.yaml`: | ||
|
|
||
| ```yaml | ||
| database: | ||
| username: "clp-user" | ||
| password: "<your-mariadb-password>" | ||
| ``` | ||
|
|
||
| :::{note} | ||
| When using external databases in a multi-host deployment, you do **not** need to start the | ||
|
|
||
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.
this has to be set as true; otherwise, the clp-config.yaml with the long URL inside would fail the rule.
@coderabbitai create an issue to track this lint rule change, and i will submit the lint rule change in a separate PR
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 42
@junhaoliao, I've created issue #1683 to track this yamllint configuration change. The issue includes the context about why this change is needed (to accommodate long URLs in
clp-config.yaml) and references this PR and comment as backlinks. The issue has been assigned to you for the follow-up PR.🧠 Learnings used