Skip to content

feat: report cloudRunReadyEvent for Cloud Run worker pools #9921

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.

Sign up for GitHub

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

Closed
ChrisGe4 wants to merge 2 commits into from
Closed

feat: report cloudRunReadyEvent for Cloud Run worker pools #9921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8fad32
Implement the `func (r runWorkerPoolResource) reportSuccess()` method…
ChrisGe4 Nov 21, 2025
61b7f39
Merge branch 'GoogleContainerTools:main' into main
ChrisGe4 Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion pkg/skaffold/deploy/cloudrun/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,4 +422,6 @@ func (r *runWorkerPoolResource) getTerminalStatus(crClient *run.APIService) (*ru
return ready, nil
}

func (r *runWorkerPoolResource) reportSuccess() {}
func (r *runWorkerPoolResource) reportSuccess() {
eventV2.CloudRunWorkerPoolReady(r.path, r.latestRevision)
}
Comment on lines +425 to +427
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This change adds new behavior but lacks corresponding tests. The existing test suite, specifically TestPrintSummaryStatus in status_test.go, does not correctly test this code path for worker pools because it uses a runServiceResource mock instead of a runWorkerPoolResource.

Please add a unit test that:

  1. Uses a runWorkerPoolResource for the test case.
  2. Asserts that eventV2.CloudRunWorkerPoolReady is called with the correct parameters when reportSuccess is invoked.

Without tests, we can't be sure this feature works as expected or protect it from future regressions.

Copy link
Contributor

@Darien-Lin Darien-Lin Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a unit test as mentioned by the bot?

Copy link
Contributor Author

@ChrisGe4 ChrisGe4 Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't added tests because there aren't any existing tests for sending the CloudRunServiceReady event, and this is a relatively simple change.

Copy link
Contributor

@Darien-Lin Darien-Lin Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these service events tested here? https://github.com/GoogleContainerTools/skaffold/blob/3b3a801033e4aae90aa4805cb1efdc216e3971f9/pkg/skaffold/deploy/cloudrun/status_test.go

Even if there aren't tests, tests should be added to guard against future regression, if anything. Having tests is helpful for any type of future development, either for refactoring or adding new functionality.

9 changes: 9 additions & 0 deletions pkg/skaffold/event/v2/cloudrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ func CloudRunServiceReady(r, url, revision string) {
})
}

func CloudRunWorkerPoolReady(r, revision string) {
handler.handleCloudRunReady(&proto.CloudRunReadyEvent{
Id: r,
TaskId: fmt.Sprintf("%s-%d", constants.Deploy, handler.iteration),
Resource: r,
ReadyRevision: revision,
})
}
Comment on lines +36 to +43
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function's implementation is very similar to CloudRunServiceReady, leading to code duplication. This can make maintenance harder. For example, if the TaskId format needs to be changed, it would have to be updated in multiple places.

To remove the duplication, you could reuse CloudRunServiceReady by passing an empty URL. This is a pragmatic way to solve the duplication within this PR. While calling a 'Service' function for a 'WorkerPool' is not ideal semantically, it avoids duplicating the event creation logic. A longer-term solution could be to introduce a more generic helper function.

func CloudRunWorkerPoolReady(r, revision string) {
	CloudRunServiceReady(r, "", revision)
}


func (ev *eventHandler) handleCloudRunReady(e *proto.CloudRunReadyEvent) {
ev.handle(&proto.Event{
EventType: &proto.Event_CloudRunReadyEvent{
Expand Down
Loading