feat: add API_EXTERNAL_URL#405
Conversation
findtopher
left a comment
findtopher
left a comment
There was a problem hiding this comment.
Looks like a good first half!
|
@kaixi-wang - can you please rebase this PR on the |
kaixi-wang
force-pushed
the
add-external-api-url
branch
from
8a28bc6 to
b2b490b
Compare
|
I made #424 which apply the suggestions + merge main. you can also commit the suggestions + rebase. Whatever works best! |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces API_EXTERNAL_URL for the teams API via Helm template logic based on apiSettings.dnsName or teamsAppSettings.dnsName. Updates README and Docker env template comments to clarify external orchestrator use. Adjusts unit tests for compose and Helm to expect the new/empty env var and correct compose file path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Helm Values
participant Tpl as Helm Template (_helpers.tpl)
participant K8s as K8s Deployment
participant API as teams-api Pod
Dev->>Tpl: Provide apiSettings.dnsName / teamsAppSettings.dnsName
Tpl->>Tpl: Compute API_EXTERNAL_URL<br/>if apiSettings.dnsName -> https://{apiDNS}<br/>else if teamsAppSettings.dnsName -> https://{appDNS}<br/>else ""
Tpl->>K8s: Render Deployment with env: API_EXTERNAL_URL
K8s->>API: Start container with env
API-->>API: Read API_EXTERNAL_URL at runtime
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
kaixi-wang
force-pushed
the
add-external-api-url
branch
from
c353158 to
a64056b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/unit/compose/docker-compose-legacy-auth_test.go (1)
30-30: Delegated-operators compose path fix is correctPointing legacyAuthComposeDelegatedOperationsFile to dockerLegacyAuthDir is consistent with this suite’s scope (legacy-auth). Nice catch.
tests/unit/helm/api-deployment_test.go (1)
436-439: LGTM: default API_EXTERNAL_URL assertions (empty) across scenariosAdding API_EXTERNAL_URL with an empty default in all baseline/env override/internal-auth/secret-name/cas override cases matches the new Helm helper logic and keeps ordering stable before CAS_BASE_URL.
Also applies to: 521-524, 620-623, 717-719, 801-803
🧹 Nitpick comments (5)
helm/fiftyone-teams-app/README.md (1)
477-477: Line-length lint violation (MD013) on updated rowThe updated description is helpful, but markdownlint flags the table row for exceeding 80 chars.
Two low-impact ways to resolve:
- Wrap the Values table with markdownlint disable/enable for
line-length:
- Insert before the table:
<!-- markdownlint-disable line-length -->- Insert after the table:
<!-- markdownlint-enable line-length -->- Or raise the per-file MD013 threshold if this table typically carries long cells.
Given other disables are already used at the top, the first option is consistent with existing style.
helm/fiftyone-teams-app/templates/_helpers.tpl (2)
340-347: Confirm path-based routing behavior; consider supporting a path prefixThe fallback to
teamsAppSettings.dnsNamecovers the host whenapiSettings.dnsNameis unset (path-based setups), but the computed value is justhttps://<host>with no path. If your ingress routes the API under a path prefix on the app host (e.g.,/api), remote orchestrators may need that prefix inAPI_EXTERNAL_URL.Options:
- Add a new optional value (e.g.,
apiSettings.externalPathPrefix) and, when set with onlyteamsAppSettings.dnsName, emithttps://<host><prefix>.- Alternatively, detect a single matching
ingress.pathsentry whoseserviceNameis the API service and append itspath(only whenapiSettings.dnsNameis empty).If you confirm your ingress never path-prefixes the API in supported deployments, feel free to ignore; otherwise, I can draft a targeted patch.
342-342: Nit: template spacing for readabilityConsistent spacing around pipelines improves readability.
Apply:
- value: {{ printf "https://%s" .Values.apiSettings.dnsName | quote}} + value: {{ printf "https://%s" .Values.apiSettings.dnsName | quote }}tests/unit/compose/docker-compose-internal-auth_test.go (1)
261-272: Good addition: asserting API_EXTERNAL_URL in teams-api envAdding
API_EXTERNAL_URLto expected env for compose, plugins, and dedicated-plugins variants aligns tests with the Helm-side change and improves parity coverage.Minor DRY suggestion: consider a helper to build the baseline expected env for
teams-api(with optional additions likeFIFTYONE_PLUGINS_DIR) to reduce duplication across these cases.Also applies to: 343-355, 425-437
tests/unit/helm/api-deployment_test.go (1)
877-953: Great precedence test; consider adding fallback-only case and input sanitization follow-up
- The overrideExternalApiUrl test correctly asserts precedence of apiSettings.dnsName over teamsAppSettings.dnsName and the https:// prefix. Nicely done.
- Suggestion: Add a test where only teamsAppSettings.dnsName is set (apiSettings.dnsName unset) to validate the intended fallback.
- Optional hardening (follow-up in chart code, not required for this PR): sanitize dnsName to avoid double schemes if a user accidentally includes “http(s)://” in values (e.g., don’t produce https://https://host). Tests could then cover this.
You can add a focused fallback test like this:
@@ func (s *deploymentApiTemplateTest) TestContainerEnv() { { "overrideExternalApiUrl", map[string]string{ "apiSettings.dnsName": "external-api-url:443", "teamsAppSettings.dnsName": "external-app-url:443", }, func(envVars []corev1.EnvVar) { expectedEnvVarJSON := `[ { "name": "API_EXTERNAL_URL", "value": "https://external-api-url:443" }, { "name": "CAS_BASE_URL", "value": "http://teams-cas:80/cas/api" }, ... ]` var expectedEnvVars []corev1.EnvVar err := json.Unmarshal([]byte(expectedEnvVarJSON), &expectedEnvVars) s.NoError(err) s.Equal(expectedEnvVars, envVars, "Envs should be equal") }, }, + { + "fallbackToTeamsAppDnsName", + map[string]string{ + // Intentionally omit apiSettings.dnsName to test fallback + "teamsAppSettings.dnsName": "external-app-url:443", + }, + func(envVars []corev1.EnvVar) { + expectedEnvVarJSON := `[ + { + "name": "API_EXTERNAL_URL", + "value": "https://external-app-url:443" + }, + { + "name": "CAS_BASE_URL", + "value": "http://teams-cas:80/cas/api" + }, + { + "name": "FIFTYONE_AUTH_SECRET", + "valueFrom": {"secretKeyRef": {"name": "fiftyone-teams-secrets", "key": "fiftyoneAuthSecret"}} + }, + { + "name": "FIFTYONE_DATABASE_NAME", + "valueFrom": {"secretKeyRef": {"name": "fiftyone-teams-secrets", "key": "fiftyoneDatabaseName"}} + }, + { + "name": "FIFTYONE_DATABASE_URI", + "valueFrom": {"secretKeyRef": {"name": "fiftyone-teams-secrets", "key": "mongodbConnectionString"}} + }, + { + "name": "FIFTYONE_ENCRYPTION_KEY", + "valueFrom": {"secretKeyRef": {"name": "fiftyone-teams-secrets", "key": "encryptionKey"}} + }, + { + "name": "MONGO_DEFAULT_DB", + "valueFrom": {"secretKeyRef": {"name": "fiftyone-teams-secrets", "key": "fiftyoneDatabaseName"}} + }, + { + "name": "FIFTYONE_ENV", + "value": "production" + }, + { + "name": "FIFTYONE_INTERNAL_SERVICE", + "value": "true" + }, + { + "name": "GRAPHQL_DEFAULT_LIMIT", + "value": "10" + }, + { + "name": "LOGGING_LEVEL", + "value": "INFO" + } + ]` + var expectedEnvVars []corev1.EnvVar + err := json.Unmarshal([]byte(expectedEnvVarJSON), &expectedEnvVars) + s.NoError(err) + s.Equal(expectedEnvVars, envVars, "Envs should be equal") + }, + },If you decide to sanitize schemes in the helm template later, I can propose a small helper to strip any leading “http://” or “https://” before prefixing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
docker/common-services.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/values.schema.jsonis excluded by!**/*.jsonhelm/fiftyone-teams-app/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
docker/internal-auth/env.template(1 hunks)docker/legacy-auth/env.template(1 hunks)helm/fiftyone-teams-app/README.md(1 hunks)helm/fiftyone-teams-app/templates/_helpers.tpl(1 hunks)tests/unit/compose/docker-compose-internal-auth_test.go(3 hunks)tests/unit/compose/docker-compose-legacy-auth_test.go(5 hunks)tests/unit/helm/api-deployment_test.go(6 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
helm/fiftyone-teams-app/README.md
477-477: Line length
Expected: 80; Actual: 456
(MD013, line-length)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: helm-unit / unit-helm
- GitHub Check: docker-unit / integration-compose-legacy
- GitHub Check: docker-unit / integration-compose-internal
- GitHub Check: helm-integration / integration-helm
- GitHub Check: docker-unit / unit-compose
- GitHub Check: pre-commit
- GitHub Check: docker-pulls
🔇 Additional comments (3)
docker/legacy-auth/env.template (1)
7-9: Comment update is clear and accurateWording now correctly calls out external orchestrators. Parity with internal-auth template is maintained.
docker/internal-auth/env.template (1)
4-6: Verify Docker Compose mapping for API_EXTERNAL_URLI didn’t find any Docker Compose YAML wiring
API_EXTERNAL_URL. Please confirm that your Compose configuration for theteams-apiservice includes:services: teams-api: environment: API_EXTERNAL_URL: ${FIFTYONE_API_URI}tests/unit/compose/docker-compose-legacy-auth_test.go (1)
261-272: VerifyAPI_EXTERNAL_URLin Docker Compose configurationsThe new unit and integration tests for both legacy-auth and internal-auth variants assert that
teams-api’s environment includesAPI_EXTERNAL_URL. Since these compose files are carried in thedocker/legacy-authanddocker/internal-authdirectories (as submodules or fixtures), please confirm that everycompose*.yamlunder those folders (compose.yaml,compose.plugins.yaml,compose.dedicated-plugins.yaml,compose.delegated-operators.yaml) has:services: teams-api: environment: - API_EXTERNAL_URL=${API_EXTERNAL_URL:-}If any are missing this entry, add it to ensure the tests pass and keep Compose and Helm in sync.
findtopher
left a comment
findtopher
left a comment
There was a problem hiding this comment.
Seems reasonable to me
4 participants
Rationale
Enabled the api to know the external api url for setting up remote orchestrators
Changes
Checklist
Testing