OTA-1975: add products service to serve products.json if present in graph-data - #1072
OTA-1975: add products service to serve products.json if present in graph-data#1072ankitathomas wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ankitathomas: This pull request references OTA-1975 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughProduct lifecycle configuration and a public ChangesProduct serving
E2E retry behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpServer
participant serve_products
participant FileSystem
Client->>HttpServer: GET /{public_app_prefix}/products
HttpServer->>serve_products: Dispatch request
serve_products->>FileSystem: Open products.json
FileSystem-->>serve_products: File or open error
serve_products-->>Client: File response or GraphError
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ankitathomas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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
🧹 Nitpick comments (4)
products-service/Cargo.toml (2)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
structoptis in maintenance mode; considerclap's derive API.structopt's features were absorbed into clap v3+ derive; structopt itself receives no new features. Since this is a new crate, using
clapdirectly would avoid starting on a maintenance-mode dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/Cargo.toml` at line 15, The dependency list is using structopt, which is now maintenance-only; replace it with clap and switch the argument parsing code to clap’s derive API. Update the relevant CLI types and derive imports that currently rely on structopt so the crate starts on clap directly rather than introducing a deprecated dependency.
8-16: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePin dependency versions exactly for reproducible builds.
All dependencies use caret ranges (
^4.4.1,^0.10,^0.4.20, etc.), allowing automatic minor/patch upgrades oncargo update. As per path instructions, "Pin exact versions; verify hashes where supported" for dependency manifests.♻️ Example fix (pin to exact versions)
- actix-web = "^4.4.1" + actix-web = "=4.4.1" commons = { path = "../commons" } - env_logger = "^0.10" + env_logger = "=0.10.0" - log = "^0.4.20" + log = "=0.4.20" - parking_lot = "^0.12" + parking_lot = "=0.12.0" - prometheus = "0.13" + prometheus = "=0.13.0" - serde_json = "^1.0.109" + serde_json = "=1.0.109" - structopt = "^0.3" + structopt = "=0.3.26"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/Cargo.toml` around lines 8 - 16, The Cargo manifest currently uses caret version ranges for dependencies like actix-web, env_logger, log, parking_lot, prometheus, serde_json, structopt, and actix-service, which allows unintentional upgrades. Update the dependency entries in products-service/Cargo.toml to exact pinned versions so builds are reproducible, keeping the existing path dependency for commons unchanged and preserving the same dependency names in the manifest.Source: Path instructions
products-service/src/config/settings.rs (1)
22-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault path duplicated with CLI default.
"/var/lib/cincinnati/graph-data/products"appears both here and as thedefault_valueincli.rs. Sincetry_mergeunconditionally overwritesdata_directoryfrom CLI (which itself defaults via structopt), thisDefaultvalue is effectively unreachable at runtime but still needs to stay in sync. Extract to a shared constant to avoid drift.♻️ Suggested fix
// in a shared location, e.g. config/mod.rs pub(crate) const DEFAULT_DATA_DIRECTORY: &str = "/var/lib/cincinnati/graph-data/products";- data_directory: PathBuf::from("/var/lib/cincinnati/graph-data/products"), + data_directory: PathBuf::from(super::DEFAULT_DATA_DIRECTORY),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/src/config/settings.rs` around lines 22 - 31, The default data directory string is duplicated between AppSettings::default and the CLI default, so move the value to a shared constant and use it in both places. Add a reusable DEFAULT_DATA_DIRECTORY in a common config location, then update AppSettings::default and the CLI argument definition in cli.rs to reference that constant so try_merge and future changes stay in sync.products-service/src/main.rs (1)
33-47: 🧹 Nitpick | 🔵 TrivialProducts data is loaded once at startup with no refresh mechanism.
If
products-lifecycle.jsonchanges on disk (e.g., graph-data update), the service won't pick it up without a restart. Consider a periodic reload similar to graph-builder's refresh pattern, especially since this data is expected to be updated externally via graph-data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/src/main.rs` around lines 33 - 47, The startup-only load in main.rs currently caches products-lifecycle.json forever, so updates on disk are never picked up. Add a periodic refresh/reload path similar to graph-builder’s refresh pattern: refactor the existing products_file/products_data loading logic in main to be reusable, then schedule a background reload that re-reads the file and swaps the in-memory JSON when it changes. Keep the current info!/warn! behavior in the reload path and ensure the service continues serving the last valid data if a refresh fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@products-service/src/main.rs`:
- Around line 52-53: The ready flag in AppState is currently dead state because
only the live health check is exposed via /healthz and nothing reads or updates
ready. Either remove the ready RwLock from main/AppState if readiness is not
needed, or implement a real readiness signal tied to products loading and add a
dedicated /readyz route that reads ready; update the handler wiring where the
state is constructed and registered so the symbol AppState reflects only used
health state.
---
Nitpick comments:
In `@products-service/Cargo.toml`:
- Line 15: The dependency list is using structopt, which is now
maintenance-only; replace it with clap and switch the argument parsing code to
clap’s derive API. Update the relevant CLI types and derive imports that
currently rely on structopt so the crate starts on clap directly rather than
introducing a deprecated dependency.
- Around line 8-16: The Cargo manifest currently uses caret version ranges for
dependencies like actix-web, env_logger, log, parking_lot, prometheus,
serde_json, structopt, and actix-service, which allows unintentional upgrades.
Update the dependency entries in products-service/Cargo.toml to exact pinned
versions so builds are reproducible, keeping the existing path dependency for
commons unchanged and preserving the same dependency names in the manifest.
In `@products-service/src/config/settings.rs`:
- Around line 22-31: The default data directory string is duplicated between
AppSettings::default and the CLI default, so move the value to a shared constant
and use it in both places. Add a reusable DEFAULT_DATA_DIRECTORY in a common
config location, then update AppSettings::default and the CLI argument
definition in cli.rs to reference that constant so try_merge and future changes
stay in sync.
In `@products-service/src/main.rs`:
- Around line 33-47: The startup-only load in main.rs currently caches
products-lifecycle.json forever, so updates on disk are never picked up. Add a
periodic refresh/reload path similar to graph-builder’s refresh pattern:
refactor the existing products_file/products_data loading logic in main to be
reusable, then schedule a background reload that re-reads the file and swaps the
in-memory JSON when it changes. Keep the current info!/warn! behavior in the
reload path and ensure the service continues serving the last valid data if a
refresh fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca59121e-0eb9-4b0b-b76c-5d9959897e2a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdist/Dockerfile.deploy/Dockerfileis excluded by!**/dist/**,!dist/**
📒 Files selected for processing (6)
Cargo.tomlproducts-service/Cargo.tomlproducts-service/src/config/cli.rsproducts-service/src/config/mod.rsproducts-service/src/config/settings.rsproducts-service/src/main.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
products-service/src/main.rs (1)
40-53: 🗄️ Data Integrity & Integration | 🟠 MajorMake readiness reflect successful product initialization.
readyis set totrueeven when reading the product file fails and{}is served, so/readyzreturns 200 while required data is unavailable. Set readiness from the load result, and validate the JSON before marking the service ready if malformed files are possible.This is the same unresolved readiness concern from the previous review; the route now exists, but the state remains unconditional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/src/main.rs` around lines 40 - 53, Update the product initialization flow in main and the shared ready state so readiness is derived from successful product-file loading rather than unconditionally set to true. Track whether the read succeeded, validate the loaded JSON before marking the service ready, and keep ready false when loading or parsing fails so /readyz reports the unavailable-data state.
🧹 Nitpick comments (1)
products-service/src/build.rs (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsume or remove the generated build metadata.
built::write_built_file()generates metadata, but the providedproducts-service/src/main.rsnever includes or references it. Either expose/use the generated constants or remove this build script and dependency to avoid unnecessary build complexity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@products-service/src/build.rs` around lines 1 - 3, Update the build metadata flow around main and the build script: either include and use the file generated by built::write_built_file in products-service/src/main.rs, or remove the build script and built dependency entirely. Ensure no unused generation remains and preserve the service’s existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@products-service/src/main.rs`:
- Around line 33-35: Align all product-file defaults with the intended location:
in products-service/src/main.rs lines 33-35, load products.json directly from
the configured graph-data directory; in products-service/src/config/settings.rs
lines 14-29, update the default directory, filename, and documentation; and in
products-service/src/config/cli.rs lines 15-20, mirror the corrected default and
help text.
---
Duplicate comments:
In `@products-service/src/main.rs`:
- Around line 40-53: Update the product initialization flow in main and the
shared ready state so readiness is derived from successful product-file loading
rather than unconditionally set to true. Track whether the read succeeded,
validate the loaded JSON before marking the service ready, and keep ready false
when loading or parsing fails so /readyz reports the unavailable-data state.
---
Nitpick comments:
In `@products-service/src/build.rs`:
- Around line 1-3: Update the build metadata flow around main and the build
script: either include and use the file generated by built::write_built_file in
products-service/src/main.rs, or remove the build script and built dependency
entirely. Ensure no unused generation remains and preserve the service’s
existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8ee29591-044b-48b6-9ff6-8922e56e3f85
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockdist/Dockerfile.deploy/Dockerfileis excluded by!**/dist/**,!dist/**
📒 Files selected for processing (7)
Cargo.tomlproducts-service/Cargo.tomlproducts-service/src/build.rsproducts-service/src/config/cli.rsproducts-service/src/config/mod.rsproducts-service/src/config/settings.rsproducts-service/src/main.rs
| // Load products data on startup | ||
| let products_file = settings.data_directory.join("products-lifecycle.json"); | ||
| let products_data = match fs::read_to_string(&products_file) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align all layers with the intended product-file path.
The effective default currently resolves to /var/lib/cincinnati/graph-data/products/products-lifecycle.json, while the PR objective specifies products.json in graph-data. This causes the service to miss the expected file and serve fallback data.
products-service/src/main.rs#L33-L35: loadproducts.jsonfrom the intended directory.products-service/src/config/settings.rs#L14-L29: update the default directory and filename documentation.products-service/src/config/cli.rs#L15-L20: mirror the corrected default and help text.
📍 Affects 3 files
products-service/src/main.rs#L33-L35(this comment)products-service/src/config/settings.rs#L14-L29products-service/src/config/cli.rs#L15-L20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@products-service/src/main.rs` around lines 33 - 35, Align all product-file
defaults with the intended location: in products-service/src/main.rs lines
33-35, load products.json directly from the configured graph-data directory; in
products-service/src/config/settings.rs lines 14-29, update the default
directory, filename, and documentation; and in
products-service/src/config/cli.rs lines 15-20, mirror the corrected default and
help text.
ankitathomas
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Signed-off-by: Ankita Thomas <[email protected]> Co-authored-by: Claude
|
/retest |
Signed-off-by: Ankita Thomas <[email protected]>
90ca277 to
9a06a3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@graph-builder/src/graph.rs`:
- Around line 153-157: Update the products file-opening error handling around
NamedFile::open to inspect the underlying I/O error kind: map
ErrorKind::NotFound to GraphError::DoesNotExist so GET /products returns 404,
while retaining GraphError::FileOpenError for permission and other failures.
Avoid consuming the result before extracting the error details.
In `@graph-builder/src/main.rs`:
- Around line 300-310: Update the serve_products_basic test to create the
fixture at the handler’s expected /var/lib/cincinnati/graph-data/products.json
location, initialize the Actix application with the products route, issue a GET
request, and assert the expected status and response body, including the
missing-file error case.
In `@hack/e2e.sh`:
- Line 42: Update the retry loop using max_attempts so its termination condition
enforces exactly 90 total executions, changing the boundary comparison to -ge
while preserving the existing backoff behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 985f121b-8204-40db-9bd5-4125c6d4975d
📒 Files selected for processing (4)
graph-builder/src/config/settings.rsgraph-builder/src/graph.rsgraph-builder/src/main.rshack/e2e.sh
| if f.is_err() { | ||
| return Err(GraphError::FileOpenError(format!( | ||
| "unable to open products.json: {}", | ||
| f.unwrap_err() | ||
| ))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return 404 when products.json is absent.
product_enabled defaults to false, so a missing products file is an expected state. Mapping every NamedFile::open failure to GraphError::FileOpenError makes GET /products return HTTP 500 instead of 404; preserve 500 for permission and other I/O failures, but map ErrorKind::NotFound to GraphError::DoesNotExist.
Proposed fix
- let f = NamedFile::open(products_path);
- if f.is_err() {
- return Err(GraphError::FileOpenError(format!(
- "unable to open products.json: {}",
- f.unwrap_err()
- )));
- }
- Ok(f.unwrap())
+ match NamedFile::open(products_path) {
+ Ok(file) => Ok(file),
+ Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+ Err(GraphError::DoesNotExist("products.json".to_string()))
+ }
+ Err(err) => Err(GraphError::FileOpenError(format!(
+ "unable to open products.json: {}",
+ err
+ ))),
+ }📝 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.
| if f.is_err() { | |
| return Err(GraphError::FileOpenError(format!( | |
| "unable to open products.json: {}", | |
| f.unwrap_err() | |
| ))); | |
| match NamedFile::open(products_path) { | |
| Ok(file) => Ok(file), | |
| Err(err) if err.kind() == std::io::ErrorKind::NotFound => { | |
| Err(GraphError::DoesNotExist("products.json".to_string())) | |
| } | |
| Err(err) => Err(GraphError::FileOpenError(format!( | |
| "unable to open products.json: {}", | |
| err | |
| ))), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graph-builder/src/graph.rs` around lines 153 - 157, Update the products
file-opening error handling around NamedFile::open to inspect the underlying I/O
error kind: map ErrorKind::NotFound to GraphError::DoesNotExist so GET /products
returns 404, while retaining GraphError::FileOpenError for permission and other
failures. Avoid consuming the result before extracting the error details.
| #[test] | ||
| fn serve_products_basic() -> Fallible<()> { | ||
| use actix_web::test; | ||
|
|
||
| // Create test products.json file | ||
| let test_data = r#"{"products":[]}"#; | ||
| let temp_dir = std::env::temp_dir(); | ||
| let products_path = temp_dir.join("test_products.json"); | ||
| std::fs::write(&products_path, test_data)?; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make serve_products_basic exercise the endpoint.
This test only writes /tmp/test_products.json and returns Ok(()); it never invokes the route, and the fixture path does not match the handler’s /var/lib/cincinnati/graph-data/products.json path. It will pass even if the endpoint is broken. Initialize the Actix app, issue a GET request, and assert the response status and body (including the missing-file case).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@graph-builder/src/main.rs` around lines 300 - 310, Update the
serve_products_basic test to create the fixture at the handler’s expected
/var/lib/cincinnati/graph-data/products.json location, initialize the Actix
application with the products route, issue a GET request, and assert the
expected status and response body, including the missing-file error case.
|
|
||
| function backoff() { | ||
| local max_attempts=60 | ||
| local max_attempts=90 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant section with line numbers.
git ls-files hack/e2e.sh
echo '---'
cat -n hack/e2e.sh | sed -n '1,140p'Repository: openshift/cincinnati
Length of output: 5634
Avoid the off-by-one retry limit. backoff allows 91 executions when max_attempts=90 because attempt starts at 0 and the loop only breaks after attempt > max_attempts. If 90 total tries are intended, switch this to -ge; otherwise rename the variable to max_retries to match the behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/e2e.sh` at line 42, Update the retry loop using max_attempts so its
termination condition enforces exactly 90 total executions, changing the
boundary comparison to -ge while preserving the existing backoff behavior.
|
@ankitathomas: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes