proposal: give identifier in KubernetesAgent.replace_job_spec_yaml() a default value

[jameslamb]

Proposal

Would you be open to a PR that makes identifier in KubernetesAgent.replace_job_spec_yaml() optional?

def replace_job_spec_yaml(
        self, flow_run: GraphQLResult, image: str, identifier: str = None
    ) -> dict:
    identifier = identifier or str(uuid.uuid4())[:8]

Background

I'm really excited about the improvements to KubernetesAgent in #3158 ! However, when trying this today I found what I think is an unnecessary breaking change.

That PR added a new keyword argument identifier to KubernetesAgent.replace_job_spec_yaml() with now default, and pulled responsibility for setting it up into KubernetesAgent.deploy_flow().

I clicked through all the discussions in that PR and I don't see any conversation about it, so I'm not sure why that change was made.

Why This is Important

I maintain a private fork of KubernetesAgent that adds some extra my-application-specific stuff in replace_job_spec_yaml(), and this change broke it in a non-backwards-compatible way. I understand that you have no responsibility to support my random fork, but if there's no compelling reason for this change, I'd like to set it back so that I don't need to add a try-except in my code to keep it backwards-compatible.

Thanks for your time and consideration!

[jameslamb]

oh I didn't realize this would end up in a "Discussion"! Is that what you'd expect for this type of thing, or should I have made other choices on the issues page so this becomes an issue?

[joshmeek]

Ah so the changes made in #3158 went through a few rounds until it ended up where it is now. That change of moving it to the deploy_flow function actually was suitable for an older iteration however I see no need for it now. We can move that identifier creation back inside the replace_job_spec_yaml function since it doesn't need to be passed around 🙂

[jameslamb]

Ok thanks! I'll make a PR shortly

[joshmeek]

Great! You'll probably have to remove the kwarg from some of the tests that call that function w/ an identifier as well and it should be good.