fix: use pointer receivers for piped protobuf models - #7174
Conversation
|
👋 Hi @srinivasr, welcome to PipeCD and thanks for opening your first pull request! We’re really happy to have you here Before your PR gets merged, please check a few important things below. Helpful resources
DCO Sign-offAll commits must include a In case you forget to sign-off your commit(s), follow these steps: For the last commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits: git rebase --signoff origin/master
git push --force-with-leaseRun checks locallyBefore pushing updates, please run: make checkThis runs the same checks as CI and helps catch issues early. 💬 Need help?If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel. Thanks for contributing to PipeCD! ❤️ |
✅ Deploy Preview for pipecd-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
i don’t think this branch compiles .. you should run |
b5ada5e to
37a0942
Compare
Generated protobuf structs such as ApplicationSyncState embed protoimpl.MessageState, which contains a sync.Mutex. Their methods used value receivers, so the lock state was copied on every call or pass, which go vet flags as a copylock violation. Switch the methods on ApplicationSyncState and ApplicationLiveStateVersion to pointer receivers and update all caller sites in driftdetector, livestatestore, livestatereporter, planpreview, and applicationlivestatestore to pass pointers. Convert range loops in the affected model tests to index loops so test cases are not copied. Signed-off-by: srinivasr <sriniv4sreddy@gmail.com>
64b5644 to
3dc472c
Compare
|
sorry for the delay on this. merged master in (it has #7137 now which touches the same HasDiff func, both changes work together fine) and squashed the branch while I was at it since the history had piled up fixups, so commit dates show today. doc comment is back and the unrelated reflector_test change is reverted as discussed also pulled the metadatastore race fix out of this PR into its own branch to keep this one focused on what was reviewed. |
|
looks good. maybe @khanhtc1202 will give a final look and merge 🙂 |

What this PR does:
generated protobuf structs like applicationsyncstate embed a sync.mutex. because their methods were using value receivers, the lock state was being copied every time they were called or passed around (flagged by go vet).
fix:
Why we need it:
passing mutexes by value creates silent data races under concurrent execution because the copies do not block each other.
Which issue(s) this PR fixes:
Fixes # N/A
Does this PR introduce a user-facing change?: