fix: execute all container hooks targeting the same pod/container#10013
fix: execute all container hooks targeting the same pod/container#10013singhlovepreet9 wants to merge 1 commit intoGoogleContainerTools:mainfrom
Conversation
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 addresses a critical bug in the deployment hook mechanism where only the initial container hook would run if multiple hooks were configured for the same pod and container. By refining the internal tracking logic to include a unique identifier for each hook, the system now correctly executes all defined container hooks, ensuring that complex deployment workflows with multiple container-level actions are fully supported. 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where multiple container hooks targeting the same pod and container would not all execute. The fix correctly resolves this by making the key used to track executed hooks unique for each hook definition by including the hook's index. The changes are logical, well-contained, and include a new unit test that effectively validates the fix by simulating the problematic scenario. The implementation is clean and directly solves the described issue.
…ogleContainerTools#8356) When multiple container hooks were configured in deploy.kubectl.hooks.after targeting the same pod and container, only the first hook would execute. The shared visitedContainers map used a key of phase:podName:containerName, causing subsequent hooks for the same container to be silently skipped. Fix by including the hook index in the visitedContainers key so each hook definition independently tracks which containers it has processed. Fixes GoogleContainerTools#8356
f1a8d76 to
c0b0cd9
Compare
Summary
Fixes #8356
containerhooks are configured indeploy.kubectl.hooks.aftertargeting the same pod and container (but with different commands), only the first hook executes. Subsequent hooks are silently skipped.visitedContainerssync.Map infilterContainersSelectoruses a key ofphase:podName:containerName. When the second hook targets the same pod/container, the key already exists and the container is filtered out.phase:hookIndex:podName:containerName) so each hook definition independently tracks which containers it has processed, while still preventing duplicate execution within the same hook across deploy iterations.Reproduction
Before this fix, only
container-hook-0would execute.Test plan
TestMultipleContainerHooksOnSamePodunit test that verifies both container hooks executeTestDeployHookscontinues to passgo test ./pkg/skaffold/hooks/ -vpasses🤖 Generated with Claude Code