Simplify zodiac team logic #288
Conversation
| @@ -1098,7 +1099,7 @@ def _set_container_labels(self, container_op: ContainerOp): | |||
| "tags.ledger.zgtools.net/ai-experiment-name", self.experiment | |||
| ) | |||
|
|
|||
| if ZILLOW_ZODIAC_SERVICE and ZILLOW_ZODIAC_TEAM: | |||
There was a problem hiding this comment.
This looks fine to me, since ZILLOW_ZODIAC_TEAM is not used within the if statement.
I don't have an understanding of how the code works together. Is there any risk if the add_pod_annotation() gets called in the case where there is no ZILLOW_ZODIAC_TEAM?
There was a problem hiding this comment.
I'm marking this PR as draft since I haven't fully worked through it. The changes here do not provide my desired result yet.
|
It looks good to me! Assuming the answer to my question above is "no", I feel good about approving. Sorry for the delayed review. |
| ret_flow_labels[f"{zodiac_prefix}/service"] = ZILLOW_ZODIAC_SERVICE | ||
| if ZILLOW_ZODIAC_TEAM: | ||
| ret_flow_labels[f"{zodiac_prefix}/team"] = ZILLOW_ZODIAC_TEAM |
There was a problem hiding this comment.
fyi: these are applied at compile time during CICD.
Should these be injected by the webhook?
There was a problem hiding this comment.
I do want to link a workflow to a zodiac service at compile time. This ensures that the workflow manifest is linked to a service no matter where it is.
I'm considering removing zodiac team label. The workflow - zodiac service association is much more stable then the workflow - zodiac team association, and zodiac team can be inferred from the zodiac service.
Why:
Zodiac service label should be added to workflows regardless of the availability of zodiac team data.
What:
Reduce logical dependency on zodiac team.