fix: emit CloudRunServiceReady event even if default url is disabled#9523
Conversation
bskaplan
force-pushed
the
disableurl
branch
from
f9eed51 to
bba880d
Compare
| if r.url != "" { | ||
| eventV2.CloudRunServiceReady(r.path, r.url, r.latestRevision) | ||
| url := r.url | ||
| if url == "" { |
There was a problem hiding this comment.
I think it'll be better to check if the service definition contains the feature flags:
run.googleapis.com/launch-stage: 'BETA'
run.googleapis.com/default-url-disabled: 'true'
There was a problem hiding this comment.
I don't want to hard-code this to that specific feature (I also definitely don't want to base it on the launch stage because that'll be removed when the feature goes GA). I think it makes more sense to trigger the event any time the service goes ready regardless of whether there's a URL or not, and as this feature shows a URL being present is not a requirement for a service going ready.
| if url == "" { | ||
| // a URL may not be present if the default URL is disabled. Use - instead of empty in case anyone is parsing the | ||
| // event status | ||
| url = "-" |
There was a problem hiding this comment.
It's not obvious that the URL can be not URL, but "dash". what if someone uses it like this:
if r.url != "" {
....
}
the condition will be true, because the URL is not empty, but contains "-"
There was a problem hiding this comment.
ok, keeping it as empty
|
@bskaplan , thank you for the contribution, I'm not the maintainer, but I left a couple of comments |
…if no URL is available
| if r.url != "" { | ||
| eventV2.CloudRunServiceReady(r.path, r.url, r.latestRevision) | ||
| } | ||
| url := r.url |
There was a problem hiding this comment.
nit: looks like we don't need this url variable anymore
|
Hey @bskaplan, thanks for helping us with this one 😄. Just a small nit, but it looks good. Just waiting for the integration test to finish. |
4 participants
Fixes: #9522
Description
Only emit CloudRunServiceReady event if the status is success, and emit it even if no URL is present which could be the case if default-url-disabled is set to true
User facing changes (remove if N/A)
Before:
A creation or update of a service with annotation run.googleapis.com/default-url-disabled: true would not emit a CloudRunServiceReady event.
If an update of an existing service failed, CloudRunServiceReady event would be emitted.
After:
A CloudRunServiceReady event is only populated if the Ready condition is successful, and is populated even if the default URL is disabled.