-
Notifications
You must be signed in to change notification settings - Fork 0
✨ Run builder when source is changed. #4
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
base: base-sha/5895e8ca60b8be2a44e00b156e71a2c0e8763da3
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
|
|
||
| from src.app import APP | ||
| from src.manager.release_manager import ReleaseManager | ||
| from src.utils import branch_name, updates_file, updates_file_url | ||
| from src.utils import app_dump_key, branch_name, updates_file, updates_file_url | ||
|
|
||
|
|
||
| class GitHubManager(ReleaseManager): | ||
|
|
@@ -34,3 +34,15 @@ def get_last_version(self: Self, app: APP, resource_name: str) -> str: | |
| if data.get(app.app_name): | ||
| return str(data[app.app_name][resource_name]) | ||
| return "0" | ||
|
|
||
| def get_last_version_source(self: Self, app: APP, resource_name: str) -> str: | ||
| """Get last patched version.""" | ||
| if os.getenv("DRY_RUN", default=None): | ||
| with Path(updates_file).open() as url: | ||
| data = json.load(url) | ||
| else: | ||
| with urllib.request.urlopen(self.update_file_url) as url: | ||
| data = json.load(url) | ||
| if data.get(app.app_name): | ||
| return str(data[app.app_name][app_dump_key][resource_name]) | ||
|
Comment on lines
+38
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code_refinement): Consider handling exceptions for JSON operations. Loading JSON data can fail due to malformed data or I/O issues. It's advisable to wrap the json.load calls in a try-except block to handle potential exceptions and maintain robustness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment helpful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment type correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment area correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What type of LLM test could this comment become?
|
||
| return "0" | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,8 +15,11 @@ def get_last_version(self: Self, app: APP, resource_name: str) -> str: | |||||||||||||||||||||||||||||||||||||||||||||||||||
| """Get last patched version.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise NotImplementedError | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def should_trigger_build(self: Self, old_version: str, new_version: str) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def should_trigger_build(self: Self, old_version: str, old_source: str, new_version: str, new_source: str) -> bool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Function to check if we should trigger a build.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if old_source != new_source: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f"Trigger build because old source {old_source}, is different from new source {new_source}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return True | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code_refinement): Refactor 'should_trigger_build' to reduce complexity. The method now handles multiple conditions and logging, which increases its complexity. Consider breaking down the functionality into smaller, more manageable methods.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment helpful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment type correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment area correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What type of LLM test could this comment become?
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider adding tests for the updated should_trigger_build method. This method now includes additional parameters and logic for source comparison. Ensure there are tests that cover various scenarios, including cases where versions are the same but sources differ, and vice versa.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this comment helpful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment type correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the comment area correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What type of LLM test could this comment become?
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.info(f"New version {new_version}, Old version {old_version}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Version(new_version) > Version(old_version) # type: ignore[no-any-return] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
suggestion (testing): Missing test cases for new source comparison logic in build triggers.
The changes include new logic to compare sources for integrations and patches to decide on triggering builds. It's crucial to add tests that verify this new functionality works as expected and handles edge cases, such as when sources are identical or when one is null.
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.
Is this comment correct?
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.
Is this comment helpful?
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.
Is the comment type correct?
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.
Is the comment area correct?
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.
What type of LLM test could this comment become?