Conversation
WalkthroughInverted feedback/transcript flags to enabling semantics, added parameters for an external insights ingress and exporter interval, created a new exporter ConfigMap, and introduced a lightspeed-to-dataverse-exporter sidecar container and related volumes/env to export feedback and transcript data externally. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainApp
participant ExporterSidecar
participant InsightsIngress
User->>MainApp: Submit feedback/transcript
MainApp->>Volume: Persist feedback/transcript files
loop Every collection_interval
ExporterSidecar->>Volume: Read allowed_subdirs (feedback, transcripts)
ExporterSidecar->>InsightsIngress: POST data (uses service_id, identity_id, auth token)
InsightsIngress-->>ExporterSidecar: Acknowledge
ExporterSidecar->>Volume: Optionally cleanup sent files
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi @keitwb. Thanks for your PR. I'm waiting for a rh-ecosystem-edge member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
template.yaml (1)
365-383: Incorrect parameter syntax –${{…}}will not be substituted by ocOpenShift template parameters use
${PARAM}. The double-brace form appears in several places and will leak into the rendered manifest.-replicas: ${{REPLICAS_COUNT}} +replicas: ${REPLICAS_COUNT} - containerPort: ${{SERVICE_PORT}} + containerPort: ${SERVICE_PORT}Same issue further down for
Service.portandRoute.targetPort.Without this fix the generated deployment is invalid.
🧹 Nitpick comments (5)
template-params.dev.yaml (1)
1-8: Consider renaming the file or adding a header to avoid format confusionThe file is named
*.yamlbut actually follows theKEY=VALUEformat expected byoc process --param-file. New contributors (or linters) may assume strict YAML and mis-edit the file.Two lightweight options:
- template-params.dev.yaml + template-params.dev.env # cleareror keep the name but add a short comment header:
+## NOTE: Not YAML – plain KEY=VALUE lines consumed by `oc process --param-file` IMAGE_TAG=latestscripts/generate.sh (2)
8-16: Quote path variables to guard against spaces
OVERRIDE_FILEmay legitimately live in a directory containing spaces, which would break the generated command line.-OVERRIDE_FILE=$PROJECT_ROOT/.template-params.override.yaml +OVERRIDE_FILE="$PROJECT_ROOT/.template-params.override.yaml"Same applies to
${OVERRIDE_PARAMS-}in theoc processinvocation.
28-35:OVERRIDE_PARAMSis used without early initialisationIf the override file does not exist,
${OVERRIDE_PARAMS-}expands to the empty string ‑ good.
When the file does exist,OVERRIDE_PARAMSis defined inside the subshell of theifblock; this still works in Bash, but explicit initialisation improves readability:-# implicit unset +OVERRIDE_PARAMS="" if [[ -f "$OVERRIDE_FILE" ]]; then - OVERRIDE_PARAMS="--param-file=$OVERRIDE_FILE" + OVERRIDE_PARAMS="--param-file=\"$OVERRIDE_FILE\"" fitemplate.yaml (2)
79-84: Description copy-paste error forINSIGHTS_DATA_COLLECTOR_ENABLEDThe description repeats the interval text and doesn’t state what the flag does.
- description: "How often to send feedback/transcript archives to the insights service" + description: "Enable/disable periodic upload of archives to the insights service"
169-172: Quoting numeric & boolean substitutions inside ConfigMap for predictable typesYAML interprets bare
true/falseas booleans and bare numbers as ints.
If any parameter ever becomes a string (e.g.,"false"vsfalse), the resulting type drift can break the application. Consider quoting all substituted values inside ConfigMaps.Example:
- feedback_enabled: ${LIGHTSPEED_FEEDBACK_ENABLED} + feedback_enabled: "${LIGHTSPEED_FEEDBACK_ENABLED}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lightspeed-stack.template.yaml(0 hunks)scripts/generate.sh(2 hunks)template-params.dev.yaml(1 hunks)template.yaml(5 hunks)
💤 Files with no reviewable changes (1)
- lightspeed-stack.template.yaml
🔇 Additional comments (1)
template.yaml (1)
64-72: Boolean parameter defaults invert behaviour between template & dev param fileTemplate default sets both
LIGHTSPEED_FEEDBACK_ENABLEDandLIGHTSPEED_TRANSCRIPTS_ENABLEDto"true"whereastemplate-params.dev.yamlsets them tofalse.
That is fine for dev ↔ prod differences, but please document this explicitly inREADMEor the parameter descriptions to avoid future confusion.
scripts/generate.sh
Outdated
| oc process --local \ | ||
| -f $PROJECT_ROOT/template.yaml \ | ||
| ${OVERRIDE_PARAMS-} \ | ||
| --param-file=$PROJECT_ROOT/template-params.dev.yaml | \ | ||
| yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ | ||
| > $PROJECT_ROOT/config/lightspeed-stack.yaml | ||
|
|
||
| yq -r '.objects[] | select(.metadata.name == "lightspeed-stack-config") | .data.system_prompt' "$PROJECT_ROOT/template.yaml" > "$PROJECT_ROOT/config/system_prompt" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
System-prompt extraction bypasses parameter substitution
system_prompt is taken straight from template.yaml, while lightspeed-stack.yaml is produced from the processed template.
If the prompt ever becomes parameterised, the two files will diverge.
Recommend extracting from the processed output as well:
-yq -r '.objects[] | select(.metadata.name == "lightspeed-stack-config") | .data.system_prompt' "$PROJECT_ROOT/template.yaml" > "$PROJECT_ROOT/config/system_prompt"
+oc process --local \
+ -f "$PROJECT_ROOT/template.yaml" \
+ ${OVERRIDE_PARAMS-} \
+ --param-file="$PROJECT_ROOT/template-params.dev.yaml" | \
+ yq -r '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data.system_prompt' \
+ > "$PROJECT_ROOT/config/system_prompt"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc process --local \ | |
| -f $PROJECT_ROOT/template.yaml \ | |
| ${OVERRIDE_PARAMS-} \ | |
| --param-file=$PROJECT_ROOT/template-params.dev.yaml | \ | |
| yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ | |
| > $PROJECT_ROOT/config/lightspeed-stack.yaml | |
| yq -r '.objects[] | select(.metadata.name == "lightspeed-stack-config") | .data.system_prompt' "$PROJECT_ROOT/template.yaml" > "$PROJECT_ROOT/config/system_prompt" | |
| oc process --local \ | |
| -f $PROJECT_ROOT/template.yaml \ | |
| ${OVERRIDE_PARAMS-} \ | |
| --param-file=$PROJECT_ROOT/template-params.dev.yaml | \ | |
| yq '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data."lightspeed-stack.yaml"' -r \ | |
| > $PROJECT_ROOT/config/lightspeed-stack.yaml | |
| oc process --local \ | |
| -f "$PROJECT_ROOT/template.yaml" \ | |
| ${OVERRIDE_PARAMS-} \ | |
| --param-file="$PROJECT_ROOT/template-params.dev.yaml" | \ | |
| yq -r '.items[] | select(.kind == "ConfigMap" and .metadata.name == "lightspeed-stack-config").data.system_prompt' \ | |
| > "$PROJECT_ROOT/config/system_prompt" |
🤖 Prompt for AI Agents
In scripts/generate.sh around lines 32 to 40, the system_prompt is extracted
directly from the unprocessed template.yaml, bypassing parameter substitution,
which can cause divergence if parameters are added. Modify the script to extract
system_prompt from the processed output (the result of oc process) instead of
the raw template.yaml, ensuring both lightspeed-stack.yaml and system_prompt
reflect the same parameter substitutions.
|
/hold We can't use the data collection yet until the SRE team merges all of the policy MRs. |
template-params.dev.yaml
Outdated
There was a problem hiding this comment.
The extension is .yaml but it doesn't seem like a YAML file. Also any reason not to use the existing .env and .env.template files for this?
There was a problem hiding this comment.
You are right, fixed the file extension.
There was a problem hiding this comment.
There seems to be a difference between referencing things in the template via the ${env.FOO} and just direct template params.
|
/ok-to-test |
keitwb
force-pushed
the
insights-config
branch
5 times, most recently
from
bd55b1f to
f2017bf
Compare
keitwb
changed the title
keitwb
force-pushed
the
insights-config
branch
4 times, most recently
from
4793533 to
0fb7dbb
Compare
keitwb
force-pushed
the
insights-config
branch
2 times, most recently
from
cecb6ee to
e752432
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template.yaml(4 hunks)
🔇 Additional comments (1)
template.yaml (1)
59-65: Default-to-“enabled” silently flips previous behaviorThe old parameters were _DISABLED with default
false; replacing them with _ENABLED and default"true"enables feedback & transcript collection in every fresh deployment. Unless this is an intentional policy change already communicated to all environments, consider defaulting to"false"(opt-in) to avoid unexpected data capture.
template.yaml
Outdated
| value: "1800" # 30 minutes | ||
| description: "How often to send feedback/transcript archives to the insights service" | ||
| - name: INSIGHTS_DATA_COLLECTOR_ENABLED | ||
| value: "true" | ||
| description: "Whether the data export of feedback/transcripts to the insights ingress server is enabled or not" |
There was a problem hiding this comment.
INSIGHTS_DATA_COLLECTOR_ENABLED is unused – side-car always runs
The template defines a flag to toggle the exporter but never references it when assembling spec.template.spec.containers. The exporter container will therefore launch even when the parameter is set to "false". Gate the container (or at least set command: ["sleep","infinity"]) behind this parameter.
🤖 Prompt for AI Agents
In template.yaml around lines 75 to 79, the INSIGHTS_DATA_COLLECTOR_ENABLED
parameter is defined but not used to conditionally include or disable the
exporter sidecar container. To fix this, modify the container specification in
spec.template.spec.containers to check the value of
INSIGHTS_DATA_COLLECTOR_ENABLED. If it is "false", either omit the exporter
container or set its command to ["sleep","infinity"] to prevent it from running.
This ensures the exporter sidecar only runs when explicitly enabled.
| feedback_enabled: ${LIGHTSPEED_FEEDBACK_ENABLED} | ||
| feedback_storage: "${STORAGE_MOUNT_PATH}/feedback" | ||
| transcripts_disabled: ${LIGHTSPEED_TRANSCRIPTS_DISABLED} | ||
| transcripts_enabled: ${LIGHTSPEED_TRANSCRIPTS_ENABLED} | ||
| transcripts_storage: "${STORAGE_MOUNT_PATH}/transcripts" | ||
| data_collector: | ||
| # We use the external collector as a sidecar | ||
| enabled: false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mismatch between internal & external collectors
user_data_collection.data_collector.enabled is hard-coded to false. If the exporter side-car is disabled (see issue above) the system will collect data to disk but never ship it. Consider wiring enabled: ${INSIGHTS_DATA_COLLECTOR_ENABLED} so both components stay consistent.
🤖 Prompt for AI Agents
In template.yaml around lines 152 to 158, the data_collector.enabled field is
hard-coded to false, causing a mismatch between internal data collection and the
external exporter sidecar. To fix this, replace the hard-coded false with a
parameter reference like ${INSIGHTS_DATA_COLLECTOR_ENABLED} so that enabling or
disabling the data collector is consistent across components.
| - name: lightspeed-to-dataverse-exporter | ||
| # TODO: change this to the official repo once it gets integrated in lightspeed build | ||
| # pipeline | ||
| image: quay.io/ometelka/lightspeed-dataverse-exporter:latest | ||
| imagePullPolicy: Always | ||
| args: | ||
| - "--mode" | ||
| - "manual" | ||
| - "--config" | ||
| - "/etc/config/config.yaml" | ||
| - "--log-level" | ||
| - "INFO" | ||
| env: | ||
| - name: INGRESS_SERVER_AUTH_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: ${INSIGHTS_INGRESS_SECRET_NAME} | ||
| key: auth_token | ||
| resources: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Side-car image is mutable latest and lives in a personal repo
quay.io/ometelka/lightspeed-dataverse-exporter:latest + the TODO comment jeopardises reproducibility and supply-chain security. Pin to a digest or versioned tag in an official org namespace before merging. Example:
- image: quay.io/ometelka/lightspeed-dataverse-exporter:latest
+ image: quay.io/lightspeed-core/lightspeed-dataverse-exporter:v0.1.0 # pinned📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: lightspeed-to-dataverse-exporter | |
| # TODO: change this to the official repo once it gets integrated in lightspeed build | |
| # pipeline | |
| image: quay.io/ometelka/lightspeed-dataverse-exporter:latest | |
| imagePullPolicy: Always | |
| args: | |
| - "--mode" | |
| - "manual" | |
| - "--config" | |
| - "/etc/config/config.yaml" | |
| - "--log-level" | |
| - "INFO" | |
| env: | |
| - name: INGRESS_SERVER_AUTH_TOKEN | |
| valueFrom: | |
| secretKeyRef: | |
| name: ${INSIGHTS_INGRESS_SECRET_NAME} | |
| key: auth_token | |
| resources: | |
| - name: lightspeed-to-dataverse-exporter | |
| # TODO: change this to the official repo once it gets integrated in lightspeed build | |
| # pipeline | |
| image: quay.io/lightspeed-core/lightspeed-dataverse-exporter:v0.1.0 # pinned | |
| imagePullPolicy: Always | |
| args: | |
| - "--mode" | |
| - "manual" | |
| - "--config" | |
| - "/etc/config/config.yaml" | |
| - "--log-level" | |
| - "INFO" | |
| env: | |
| - name: INGRESS_SERVER_AUTH_TOKEN | |
| valueFrom: | |
| secretKeyRef: | |
| name: ${INSIGHTS_INGRESS_SECRET_NAME} | |
| key: auth_token | |
| resources: |
🤖 Prompt for AI Agents
In template.yaml around lines 450 to 468, the sidecar image uses the mutable
'latest' tag from a personal repository, which risks reproducibility and
supply-chain security. Replace the image tag with a fixed versioned tag or a
digest from an official organization namespace, and update the TODO comment
accordingly to reflect this change before merging.
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
template.yaml (4)
149-156: Internal/external collector mismatch remains
user_data_collection.data_collector.enabledis still hard-coded tofalse, so the application writes feedback/transcripts to disk while the in-process collector is disabled. If the exporter side-car is also gated (see previous comment) nothing will ever ship the data. Wire this flag to the same parameter you use to control the exporter so they stay consistent.
450-451: Mutablelatesttag from a personal repository – pin the image
Usingquay.io/ometelka/lightspeed-dataverse-exporter:latestcompromises reproducibility and supply-chain security. Pin to a digest or a versioned tag in an official org namespace before you merge.
65-76: Exporter cannot be disabled – introduce anINSIGHTS_EXPORTER_ENABLEDflag
The template adds multiple Insights-related parameters but never provides a way to disable the exporter. Given the SRE “/hold” until policies are merged, the inability to turn the side-car off is risky (PII leakage / policy breach).Add a boolean parameter and reference it in the container block (see lines 447-478).
+# Controls whether the dataverse exporter side-car is started +- name: INSIGHTS_EXPORTER_ENABLED + value: false + description: "Enable the lightspeed-to-dataverse exporter side-car"Then gate the container with a simple entry-point guard (or omit the container entirely when
false).
447-478: Side-car always runs regardless of policy flag – potential PII leak
Because the exporter container is unconditionally listed, it will start in every environment, even where data export is forbidden. Until the SRE policies are merged this must be suppressible.Minimal workaround (assuming you add
INSIGHTS_EXPORTER_ENABLED, defaultfalse):env: + - name: INSIGHTS_EXPORTER_ENABLED + value: ${INSIGHTS_EXPORTER_ENABLED} @@ - args: - - "--mode" - - "manual" - - "--config" - - "/etc/config/config.yaml" - - "--log-level" - - "INFO" + command: + - /bin/bash + - -c + - | + if [[ "${INSIGHTS_EXPORTER_ENABLED}" == "true" ]]; then + exec /usr/local/bin/lightspeed-dataverse-exporter \ + --mode manual \ + --config /etc/config/config.yaml \ + --log-level INFO + else + exec sleep infinity + fiThis prevents the binary from executing unless explicitly enabled.
🧹 Nitpick comments (2)
template.yaml (2)
59-65: Prefer un-quoted booleans for template parameters
Quoting the default values ("true") turns them into strings. Several downstream components (especially Helm/YAML-parsed configs) expect a real boolean and will silently treat the string"true"as truthy but not boolean, which can break.Valuesinterpolation and logical checks.
Remove the quotes so the parameters expand to the native YAML boolean.- value: "true" + value: true
542-543:collection_intervalshould be numeric
The parameter default is quoted, so the rendered YAML becomes a string ("1800"). If the exporter strictly parses integers this will fail at start-up. Drop the quotes in the parameter default or wrap the value here in!!intto force YAML integer typing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
template.yaml (4)
450-451: Mutablelatesttag from a personal repo – still unresolved
The image referencequay.io/ometelka/lightspeed-dataverse-exporter:latestrepeats the supply-chain & reproducibility concerns flagged in the previous review. Pin to a digest or versioned tag in an official org namespace before shipping.
447-478: Side-car unconditionally runs ‒ gate it or sleep
Same issue raised previously: the exporter container is inserted regardless of whether the feature is enabled.
A minimal fix that keeps the template simple is to wrap the real command in a shell gate:- args: - - "--mode" - - "manual" - - "--config" - - "/etc/config/config.yaml" - - "--log-level" - - "INFO" + command: + - /bin/sh + - "-c" + - | + if [ "${INSIGHTS_DATA_COLLECTOR_ENABLED}" = "true" ]; then + exec /usr/local/bin/lightspeed-dataverse-exporter \ + --mode manual \ + --config /etc/config/config.yaml \ + --log-level INFO + else + echo "Insights exporter disabled"; sleep infinity + fi + env: + - name: INSIGHTS_DATA_COLLECTOR_ENABLED + value: ${INSIGHTS_DATA_COLLECTOR_ENABLED}
66-76: Missing master toggle for the exporter side-car
You introduce several parameters for exporter connectivity, but there is still no boolean such asINSIGHTS_DATA_COLLECTOR_ENABLED(or similar) to decide whether the side-car should even be started.
Without it, clusters that do not have the secret or external approval will still launch a container that loops and fails.+# - name: INSIGHTS_DATA_COLLECTOR_ENABLED +# value: "false" +# description: "Set to true to start the insights exporter side-car"
149-156: Keep internal & external collectors consistent
data_collector.enabledis hard-coded tofalsewhile the exporter side-car is always present. Couple this field to the (missing) master toggle so the YAML cannot get out of sync:- enabled: false + enabled: ${INSIGHTS_DATA_COLLECTOR_ENABLED}
🧹 Nitpick comments (2)
template.yaml (2)
483-485: Tighten ConfigMap volume permissions
Mount the exporter config read-only to prevent accidental mutation from the container:- - name: lightspeed-exporter-config - configMap: - name: lightspeed-exporter-config + - name: lightspeed-exporter-config + configMap: + name: lightspeed-exporter-config + defaultMode: 0440
532-539: YAML list indentation is off by one space
The items underallowed_subdirsmust be indented two spaces deeper than the key to be valid YAML.- allowed_subdirs: - - feedback - - transcripts + allowed_subdirs: + - feedback + - transcripts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template.yaml(4 hunks)
🔇 Additional comments (1)
template.yaml (1)
59-65: Positive-logic flags are clearer – good move
Switching from “_DISABLED” to “_ENABLED” parameters removes the double-negative mental overhead and aligns with Kubernetes style conventions.
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
/lgtm
Just the exporter image issue.
| - name: lightspeed-to-dataverse-exporter | ||
| # TODO: change this to the official repo once it gets integrated in lightspeed build | ||
| # pipeline | ||
| image: quay.io/ometelka/lightspeed-dataverse-exporter:latest |
There was a problem hiding this comment.
I don't think we can use this image in app sre env
There are several PRs that the SRE and Insights team need to approve before this will work locally or in staging but it should give us most of the config we would need to use this feature and start collecting the feedback/transcript data for research.
keitwb
force-pushed
the
insights-config
branch
from
30a136e to
c0febba
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, keitwb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
There are several PRs that the SRE and Insights team need to approve before this will
work locally or in staging but it should give us most of the config we would need
to use this feature and start collecting the feedback/transcript data for research.
Summary by CodeRabbit
New Features
Chores