[OTA-2089] fall back to local source for plc_lookup - #45
Conversation
65e5b71 to
ee6eb76
Compare
caf8fcf to
c6b66eb
Compare
c6b66eb to
a8fabb4
Compare
jrangelramos
left a comment
There was a problem hiding this comment.
Just a few nits. Also perhaps data/product.json should be back to the repo (and later addressed/updated via PR with github action or something similar)
1471a6f to
384507c
Compare
jrangelramos
left a comment
There was a problem hiding this comment.
Hi Ankita! Thanks for the fallback code. I've tested with many combinations using both live and fallback data and it looks it mirrors the search logic properly. Nice job! Just a note about connectivity check, it looks redundant and can be simplified.
| API_BASE = "https://access.redhat.com/product-life-cycles/api/v2/products" | ||
| PRODUCTS_PATH = os.path.join(os.path.dirname(__file__), "data", "products.json") | ||
|
|
||
| def check_connectivity(url, timeout=5): |
There was a problem hiding this comment.
I think this could be simplified. The same checks are already performed on the api_search so maybe you can just call the fallback function in the exception handling of the api_search. Having this check also makes the live api to be called twice in a happy path where API is available. So perhaps, consider removing it.
There was a problem hiding this comment.
Good catch, moved the fallback logic to when the api is unreachable
|
Hello @ankitathomas the test |
cc4a71c to
abb7269
Compare
|
/lgtm |
|
/verified by @jrangelramos Ran locally a bunch of lookup test and compared results with live api. |
|
@jrangelramos: This PR has been marked as verified by 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. |
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
abb7269 to
cb21827
Compare
|
[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 |
|
@ankitathomas: The following test failed, say
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. |
|
/lgtm |
| import urllib.request | ||
|
|
||
| API_BASE = "https://access.redhat.com/product-life-cycles/api/v2/products" | ||
| PRODUCTS_PATH = os.path.join(os.path.dirname(__file__), "data", "products.json") |
There was a problem hiding this comment.
This file isn't committed so wouldn't be in the final image built from this repo. Is there any plan for this to be committed and/or mounted into wherever this will be run?
| if "name" in i and n in i["name"].lower(): | ||
| names.append(i) |
There was a problem hiding this comment.
let's do some deduplication here, otherwise we can end up with the same product twice (or maybe there is some structure to this data that prevents this, but it is not clear from this PR/comment that there is).
For example, something like name="mcp,openshift" with a product name `"MCP Server for Red Hat OpenShift" would end up matching twice
| url = API_BASE | ||
| if name: | ||
| url = f"{url}?{urllib.parse.urlencode({'name': name})}" | ||
| url = f"{url}?{urllib.parse.urlencode({'name': name})}&match_mode=contains" |
There was a problem hiding this comment.
shouldn't we urlencode the match_mode=contains as well?
| self.assertEqual(result[0]["name"], "logging for Red Hat OpenShift") | ||
| finally: | ||
| os.unlink(temp_path) | ||
| def test_former_names_fallback_logic(self): |
There was a problem hiding this comment.
doesn't this test case make more sense in TestSearchLocal ?
|
|
||
|
|
||
| def api_search(name=None): | ||
| def search_local(name=None, path=PRODUCTS_PATH): |
There was a problem hiding this comment.
nit: we should add one more blank line above this function, PEP 8 requires two blank lines between top-level function definitions
Include product lifecycle data as a file in agentic-skills image and have plc_lookup.py use the local file if the products lifecycle API is unreachable, such as in a disconnected environment.