Add tests, improve code quality and support compose lock file#85
Add tests, improve code quality and support compose lock file#85iignatevich wants to merge 1 commit into
Conversation
iignatevich
commented
Mar 23, 2026
iignatevich
commented
- Add unit and integration tests for sync logic
- Add compose lock file support
- Add make targets: install-bin, test-race, test-integration
- Improve code quality
- Fix race condition: use local errR instead of shared err variable in goroutine closure (findResourcesChangeTime)
- Replace buildHackAuthor sentinel string with explicit overridden bool field in hashStruct
- Remove dead commented-out code (GetChangedResources, getMapKeys, maxWorkers, list recursion)
- Add todo comments for operator precedence bug in extractLinesWithVariables and strings.Contains error matching
- Add unit and integration tests for sync logic - Add compose lock file support - Add make targets: install-bin, test-race, test-integration - Improve code quality - Fix race condition: use local errR instead of shared err variable in goroutine closure (findResourcesChangeTime) - Replace buildHackAuthor sentinel string with explicit overridden bool field in hashStruct - Remove dead commented-out code (GetChangedResources, getMapKeys, maxWorkers, list recursion) - Add todo comments for operator precedence bug in extractLinesWithVariables and strings.Contains error matching
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on bolstering the project's robustness and developer experience. It introduces a thorough testing suite for the synchronization mechanism, integrates modern dependency management with compose lock files, and provides new utility Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the project's quality and functionality. You've added a comprehensive suite of unit and integration tests, which is a huge step forward for maintainability and stability. The support for compose.lock files is a great enhancement.
The code quality improvements are also very valuable, especially the fix for the race condition in findResourcesChangeTime and the refactoring to remove the buildHackAuthor sentinel string in favor of a clearer overridden boolean flag. The removal of dead code and the addition of new Makefile targets are also welcome changes.
I have one suggestion regarding a bug you've already identified with a @todo comment. I believe it's better to fix it now to prevent potential issues with variable dependency analysis. See my specific comment for details.
Overall, great work on this PR!
| // parentheses around the OR condition. Fix: add parentheses around (Contains("{{") || Contains("}}")). | ||
| // Before fixing, verify there are no real cases where {{ and }} appear on separate lines | ||
| // in Ansible templates (e.g. multiline Jinja2 expressions), as the fix would break those. | ||
| if len(line) > 0 && !strings.HasPrefix(line, "#") && strings.Contains(line, "{{") || strings.Contains(line, "}}") { |
There was a problem hiding this comment.
You've correctly identified an operator precedence bug here. The current logic ... && strings.Contains(line, "{{") || strings.Contains(line, "}}") will incorrectly process commented-out lines if they contain }}, because && has higher precedence than ||. This could lead to incorrect variable dependency analysis.
I recommend fixing this now by adding parentheses as you suggested in the @todo: (strings.Contains(line, "{{") || strings.Contains(line, "}}")).
Regarding your concern about multiline Jinja2 expressions, this change should handle them correctly. It will include any non-comment line that is part of a multiline expression (i.e., contains {{ or }}), which seems to be the desired behavior.
| if len(line) > 0 && !strings.HasPrefix(line, "#") && strings.Contains(line, "{{") || strings.Contains(line, "}}") { | |
| if len(line) > 0 && !strings.HasPrefix(line, "#") && (strings.Contains(line, "{{") || strings.Contains(line, "}}")) { |
1 participant
