-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENH] Load pipeline catalog dynamically and error out when it is empty/not found #391
Conversation
Needs new requests dependency
No longer needed
why did this just work locally?
Must also be updated of course
If only pre-commit installed automatically for me ...
Pull Request Test Coverage Report for Build 11940477817Details
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
+ Coverage 98.36% 98.42% +0.06%
==========================================
Files 18 18
Lines 977 1016 +39
==========================================
+ Hits 961 1000 +39
Misses 16 16 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
@alyssadai this should be ready for you now |
Reviewer's Guide by SourceryThis PR implements dynamic loading of the pipeline catalog by first attempting to fetch it from a remote GitHub URL, with a fallback to a local backup file if the remote fetch fails. The implementation uses the Sequence diagram for dynamic loading of pipeline catalogsequenceDiagram
participant User
participant CLI
participant RemoteServer
participant LocalBackup
User->>CLI: Request pipeline catalog
CLI->>RemoteServer: Fetch catalog from remote URL
alt Remote fetch successful
RemoteServer-->>CLI: Return catalog data
else Remote fetch fails
CLI->>LocalBackup: Load catalog from local backup
LocalBackup-->>CLI: Return catalog data
end
CLI-->>User: Provide catalog data
Updated class diagram for pipeline catalog loadingclassDiagram
class Mappings {
+Namespace NP
+Path PROCESSING_PIPELINE_PATH
+String PROCESSING_PIPELINE_URL
+parse_pipeline_catalog()
+get_pipeline_catalog(get_url: str, get_path: Path) dict
+get_pipeline_uris(in_arr: list) dict
+get_pipeline_versions(in_arr: list) dict
}
class FileUtils {
+load_json(path: Path) dict
}
Mappings --> FileUtils : uses
note for Mappings "Handles dynamic loading of pipeline catalog"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @surchs - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using the
responses
library for mocking HTTP requests in tests - it provides a cleaner way to mock requests and responses without the current workarounds needed for httpx.Response objects.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Hi @surchs! 👋
@sourcery-ai is now installed on this repository.
We found this recent PR of yours and reviewed it to show you what Sourcery can do.
If you want to review another PR, just comment with @sourcery-ai review
✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @surchs!
The overall logic of the changes makes sense to me. I left some suggestions and a few comments for code clarity, and I think there's one potential modification to be made to ensure we don't emit an irrelevant error message by using one of the existing CLI utilities.
See what makes sense to you!
Thanks Alyssa, agree with all your comments. Unfortunately airport network is too bad for me to accept your changes, so if you'd just push directly to this branch and then accept for me, I'd appreciate! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧑🍳
🚀 PR was released in |
No longer needed
pipeline-catalog
should be fetched dynamically #38350ffffc
to841396c
#386pipeline-catalog
not found or returns empty index of pipelines #374Changes proposed in this pull request:
requests
to load the catalogFor reviewer: seems like mocking
requests
is a whole thing. My unit test mainly ensures that our fallback local file works if the request to the GitHub pipeline catalog fails. But we may want to be a bit smarter about testing this in the future. Maybe you have ideas.Checklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)skip-release
(to be applied by maintainers only)Closes #XXXX
For new features:
For bug fixes:
Summary by Sourcery
Enhance the CLI to dynamically load the pipeline catalog from a remote source, with a fallback to a local copy if the remote is unavailable. Update the project dependencies to include 'httpx' for HTTP requests and add tests to ensure the robustness of the catalog loading process.
Enhancements:
Build:
Tests: