Appending server_url for project_id resolve#101
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideNormalizes the Ibutsu server URL for API calls and adds graceful handling when a project name cannot be resolved via the Ibutsu API, falling back to the original project value while logging a warning. Sequence diagram for project_uuid resolution with 404 handlingsequenceDiagram
actor Tester
participant PytestIbutsuPlugin
participant Configuration
participant ApiClient
participant ProjectApi
Tester ->> PytestIbutsuPlugin: request project_uuid
activate PytestIbutsuPlugin
PytestIbutsuPlugin ->> PytestIbutsuPlugin: normalize ibutsu_server to server_url
PytestIbutsuPlugin ->> Configuration: create(access_token, server_url)
activate Configuration
Configuration -->> PytestIbutsuPlugin: config
deactivate Configuration
PytestIbutsuPlugin ->> ApiClient: create(config)
activate ApiClient
ApiClient -->> PytestIbutsuPlugin: api_client
deactivate ApiClient
PytestIbutsuPlugin ->> ProjectApi: create(api_client)
activate ProjectApi
PytestIbutsuPlugin ->> ProjectApi: get_project_list(filter name=project_value)
alt Project name not found
ProjectApi -->> PytestIbutsuPlugin: raise NotFoundException
PytestIbutsuPlugin ->> PytestIbutsuPlugin: log warning
PytestIbutsuPlugin -->> Tester: return project_value
else Project list returned
ProjectApi -->> PytestIbutsuPlugin: response with projects
PytestIbutsuPlugin ->> PytestIbutsuPlugin: check projects list
PytestIbutsuPlugin -->> Tester: return resolved_uuid from first project id
end
deactivate ProjectApi
deactivate PytestIbutsuPlugin
Class diagram for updated project_uuid resolution logicclassDiagram
class PytestIbutsuPlugin {
- string ibutsu_token
- string ibutsu_server
+ string project_uuid()
}
class Configuration {
+ Configuration(access_token, host)
}
class ApiClient {
+ ApiClient(configuration)
}
class ProjectApi {
+ ProjectApi(api_client)
+ get_project_list(filter)
}
class NotFoundException {
}
PytestIbutsuPlugin --> Configuration : creates
Configuration --> ApiClient : used_by
ApiClient --> ProjectApi : passed_to
PytestIbutsuPlugin --> ProjectApi : uses
ProjectApi --> NotFoundException : may_raise
Flow diagram for normalized server_url and project_id resolutionflowchart TD
A[Start project_uuid resolution] --> B[Read ibutsu_server]
B --> C{Ends with /?}
C -- Yes --> D[Remove trailing /]
C -- No --> E[Keep as is]
D --> F{Ends with /api?}
E --> F
F -- No --> G[Append /api]
F -- Yes --> H[Keep server_url]
G --> I[Create Configuration with access_token and normalized server_url]
H --> I
I --> J[Create ApiClient and ProjectApi]
J --> K[Call get_project_list with filter name=project_value]
K --> L{NotFoundException raised?}
L -- Yes --> M[Log warning Could not find project_value on server]
M --> N[Return original project_value]
L -- No --> O{response.projects exists and length > 0?}
O -- Yes --> P[resolved_uuid = first project id]
P --> Q[Return resolved_uuid]
O -- No --> R[Return project_value]
N --> S[End]
Q --> S
R --> S
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
server_urlnormalization assumes the API root ends exactly with/api, which would break hosts that already use a versioned path like/api/v1; consider usingurllib.parseto inspect/normalize the path more robustly instead of relying onendswith('/api'). - When falling back to returning
project_valueonNotFoundException, it might be helpful to include theserver_urlin the warning log so users can more easily diagnose misconfigured endpoints or typos in the project name.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `server_url` normalization assumes the API root ends exactly with `/api`, which would break hosts that already use a versioned path like `/api/v1`; consider using `urllib.parse` to inspect/normalize the path more robustly instead of relying on `endswith('/api')`.
- When falling back to returning `project_value` on `NotFoundException`, it might be helpful to include the `server_url` in the warning log so users can more easily diagnose misconfigured endpoints or typos in the project name.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (54.54%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 68.65% 68.15% -0.51%
==========================================
Files 6 7 +1
Lines 820 829 +9
Branches 140 140
==========================================
+ Hits 563 565 +2
- Misses 230 235 +5
- Partials 27 29 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3f41a70 to
eb6ff3d
Compare
|
Second issue with this 'feature' - the URL provided for IBUTSU_MODE is the API endpoint, which may be a different CNAME. Consider: including Not blocking for this MR, should be a separate scope. |
d179e56 to
e20c291
Compare
e20c291 to
08f2e42
Compare
|
Changing in ibutsu client Configuration class manually isnt allowed as it is generated by the regeneration script. Changing that would be of no use as next regeneration would revert it. We can change the regeneration script, but maybe thats too big of a change? |
To solve this
Summary by Sourcery
Resolve Ibutsu project UUIDs against the API endpoint instead of the raw server URL and handle missing projects gracefully.
Bug Fixes:
/apiendpoint when resolving project IDs.