Improve Dependency Resolution for Workflow Optimization #2355
Conversation
WalkthroughThe suite of changes enhances the Git Changes Action, focusing on optimizing dependency resolution within Go projects. By introducing package-level dependency detection and refining the action's ability to discern relevant module changes, these updates aim to streamline workflow triggers, ensuring they're only activated by pertinent modifications. Changes
Assessment against linked issues
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (3)
- contrib/git-changes-action/detector/depgraph.go (1 hunks)
- contrib/git-changes-action/detector/detector.go (2 hunks)
- contrib/git-changes-action/main.go (2 hunks)
Additional comments: 6
contrib/git-changes-action/main.go (2)
- 44-44: The addition of the
"packages"argument tooutputModuleChangesfunction calls aligns with the PR's objectives to enhance dependency resolution by focusing on package-level changes. This is a necessary update for the intended functionality.Also applies to: 49-49
- 64-64: The update to the
outputModuleChangesfunction signature, addingtypeOfDependency string, is essential for supporting the new package-level dependency tracking. This change is well-integrated and necessary for the functionality being introduced.contrib/git-changes-action/detector/depgraph.go (2)
- 21-21: The addition of the
typeOfDependencyparameter togetDependencyGraphis crucial for enabling the differentiation between module and package dependencies, aligning with the PR's objectives. This change is well-implemented.- 57-115: The logic for handling different types of dependencies ("module" and "packages") within
getDependencyGraphis well-structured and aligns with the PR's goal of enhancing dependency resolution. The conditional processing based on thetypeOfDependencyparameter is effectively implemented, ensuring accurate dependency tracking.contrib/git-changes-action/detector/detector.go (2)
- 23-50: The introduction of the
identifyNestedDependencyChangefunction is well-executed, providing necessary functionality for recursively identifying changes in nested dependencies. This addition significantly enhances the granularity of dependency tracking, aligning with the PR's objectives.- 70-125: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [53-122]
The updates to
DetectChangedModules, incorporating logic to handle "modules" and "packages" types of dependencies, are well-implemented. These changes are essential for the PR's goal of optimizing workflow triggers by focusing on package-level changes, thereby enhancing the efficiency and accuracy of dependency tracking.
avoids redundant recursion on previously analyzed packages
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/detector.go (2 hunks)
Additional comments: 2
contrib/git-changes-action/detector/detector.go (2)
- 23-42: The function
identifyNestedDependencyChangeis introduced to recursively identify changes in nested dependencies. While the logic appears sound, there are a couple of points to consider:
- Performance: Recursive calls can be expensive, especially for large dependency graphs. Consider adding a mechanism to prevent re-evaluation of packages that have already been checked.
- Documentation: Adding comments to explain the function's purpose, parameters, and return value would improve readability and maintainability.
Consider implementing memoization to avoid redundant checks on packages that have already been evaluated. Additionally, enhance the function's documentation for clarity.
- 62-117: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [45-114]
In the
DetectChangedModulesfunction, the handling of different types of dependencies (modulesorpackages) based on thetypeOfDependencyparameter is a significant change. A few observations and suggestions:
- Error Handling: The function returns early if it encounters an error, which is good practice. However, consider logging these errors for easier debugging.
- Complexity: The function's complexity has increased with the addition of package-level dependency tracking. Consider refactoring to reduce complexity and improve readability.
- Consistency: Ensure that the handling of
modulesandpackagesis consistent and that any edge cases are accounted for.To improve maintainability, consider breaking down the function into smaller, more focused sub-functions. Additionally, ensure comprehensive error logging and review the consistency of handling different dependency types.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/detector.go (2 hunks)
Additional comments: 2
contrib/git-changes-action/detector/detector.go (2)
- 23-48: The function
identifyNestedDependencyChangeis well-structured for recursively identifying nested dependency changes. However, there are a few points to consider:
- Performance: The recursive approach might lead to performance issues for deeply nested dependencies or large dependency graphs. Consider implementing memoization to avoid re-processing packages that have already been evaluated.
- Error Handling: Currently, there's no error handling for cases where
packageNamemight not exist indepGraph. Adding a check for the existence ofpackageNameindepGraphbefore proceeding with the recursion could prevent potential runtime panics.- Documentation: Adding comments to explain the function's logic, especially around the use of the
packagesmap for caching results, would improve readability and maintainability.Consider adding memoization to improve performance and error handling for non-existent package names in the dependency graph.
- 68-130: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [51-127]
In the
DetectChangedModulesfunction:
- Error Handling and Validation: The function starts by checking for the existence of a
go.workfile but does not validate thetypeOfDependencyparameter. Given that the function's behavior significantly depends on this parameter, adding validation for expected values ("modules"or"packages") could prevent unexpected behavior.- Complexity: The function's complexity has increased with the addition of handling for package dependencies. Consider refactoring to separate concerns, such as extracting the logic for module and package dependency handling into separate functions. This would not only reduce complexity but also improve the function's readability and maintainability.
- Documentation: The function could benefit from more detailed comments, especially explaining the new logic introduced for handling package dependencies. This would aid in understanding the changes and the rationale behind them.
Refactor to improve error handling, reduce complexity, and enhance documentation. Consider validating
typeOfDependencyand extracting logic into smaller, focused functions.
|
@oliverqx btw working on some changes to let these wokflows run correctly |
Step 1: 1. Get all packages per module 2. Get all package names publicName<->relativeName Step 2: 1. Extracts dependencies This is in preparation for the next commit which will make dependencies per package a set (to avoid dupllicates), and will add dependency filtering to step 2, so that only synapsecns/sanguine packages are included
Changed from map[string][]string to map[string]map[string]struct{}. Will essentially work as a set to avoid any dependency duplication
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/depgraph.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/detector/depgraph.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/depgraph.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/detector/depgraph.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/depgraph.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/detector/depgraph.go
nested packages whose directory name is found in the module's name, were overriding the module's dependencyNames entry, this commit fixes that issue
dependency filtering is now taken care of on dependency map creation
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/detector/depgraph.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/detector/depgraph.go
Looking into this, quite a strange bug |
|
Would you let me know if the README.md is accurate and understandable cc: @trajan0x |
|
@trajan0x today im pretty much free, will be looking into that bug. I think Its got to do with ct.HasPath() might need to do some edits there, will keep you updated |
|
Appreciate it |
|
@trajan0x Actually I realized its not a bug. I created this PR in my own sanguine replica: https://github.com/oliverqx/sanguine-git-changes/pull/4 And I added an fmt.Println to all ./contrib/promexporter dependencies to my image and it looks like ./ethergo/submitter is one of the dependencies. Keep in mind that this array a contains all transitive dependencies, so manually following it might be quite complicated.
Edit: To manually folllow:
Current solution will render a module as changed if any of the packages within its dependency tree is changed |
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/README.md (5 hunks)
Additional comments not posted (1)
contrib/git-changes-action/README.md (1)
97-138: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-138]
Review the entire documentation for clarity and consistency.
Ensure that the documentation accurately reflects the changes made in the PR, particularly the transition to package-based dependency tracking. Verify that examples and usage instructions are up-to-date and align with the new functionality introduced.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/main.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/main.go
|
@trajan0x just checking in, I think the solution is ready. Please let me know is there's anything else I could do |
|
Okay, will stage the latest build today |
There was a problem hiding this comment.
Actionable comments posted: 4
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- contrib/git-changes-action/README.md (5 hunks)
- contrib/git-changes-action/main.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/main.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: .coderabbit.yaml
Files selected for processing (1)
- contrib/git-changes-action/main.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- contrib/git-changes-action/main.go
|
staged in #2444 Assuming no snafus for next day im good to gtm |
|
sounds good, thanks! |
|
@trajan0x once the bounty has been settled, I can provide support if anything is wrong. I know my solution is right, but nonetheless. Was also wondering if I could get a linkedin review or a recommendation for my personal blog. Would greatly appreciate your input |
|
Absolutely. Wanna dm me somewhere? |
|
can i get your discord? |
|
@trajan0x my discord is @olxverqx, or oliverquixchan@gmail.com |
/claim #2235
Description
This PR implements module package-based dependency resolution as requested by issue #2235.
Additional context
Linked issue explains it quite well.
Metadata
Summary by CodeRabbit
Git Changes Actionto export a variable with changed Go modules in pull requests, introducingModule to moduleandModule to packagedependency resolution methods.levelOfDependencyResolutionfor specifying resolution method and examples for testing based on modified modules and dependencies.