https://prefect.io logo
Title
d

David Beck

10/19/2022, 5:10 PM
Hi team! I'm building out a deployment flow for our CI/CD purposes for k8s and was looking to use the the
KubernetesJob.job_from_file
function to read a yaml file with our full k8s manifest. The function works, however I noticed that we have fields that get overwritten with default values in this call for
_shortcut_customizations
in
build_job
, specifically the
namespace
and
image
properties. I know that I can use an
infrastructure_override
when setting up the deployment, however there should be option to simply read a yaml file with those fields.
1
z

Zanie

10/19/2022, 5:12 PM
Hey! Let me rope in the engineer who worked on this with me 🙂 @Chris Guidry do you remember the choices we made here?
@David Beck What if you explicitly set
namespace
and
image
to
None
?
d

David Beck

10/19/2022, 5:15 PM
I get this error:
pydantic.error_wrappers.ValidationError: 2 validation errors for KubernetesJob
image
  none is not an allowed value (type=type_error.none.not_allowed)
namespace
  none is not an allowed value (type=type_error.none.not_allowed)
z

Zanie

10/19/2022, 5:19 PM
I see. We should probably allow null values for those and then error if they are not present in the file (manifest).
or default them to
None
and only set a value for them if they are not present in the manifest (probably the better path forward).
☝️ 1
1
d

David Beck

10/19/2022, 5:20 PM
or allow for the values of the manifest to populate the class attributes during the build
c

Chris Guidry

10/19/2022, 5:23 PM
Oh gosh, I don't think we intended to overwrite what was in the manifest that way. I think we intended to have
image
and
namespace
be nullable in the same way the other "shortcuts" are. I recall it was tricky because we were trying maintain backwards compatibility for those two attributes. I feel like allowing
None
for those would work, and we can make the shortcut function apply those conditionally just like the others
d

David Beck

10/19/2022, 5:26 PM
Well I'm glad I brought this your attention! I at least have a few known workarounds to apply values successfully, but I do think the ideal would be to allow those to set in the manifest. Would either of you be able to report the issue on my behalf?
c

Chris Guidry

10/19/2022, 5:29 PM
Sure thing, I can take care of that. In the meantime, one workaround could be something like:
job = KubernetesJob.job_from_file(...)
KubernetesJob(
  image=job['metadata']['namespace'],
  image=job['spec']['template']['spec']['containers'][0]['image']
  ...,
)
(depending on how the job is structured, naturally)
1
z

Zanie

10/19/2022, 5:30 PM
@Marvin open “Kubernetes manifest image/namespace are overridden by default shortcuts”
:wizard2: 1
We’ll see if Marvin is a good bot today.
💔 1
c

Chris Guidry

10/19/2022, 5:32 PM
That’s okay, I can write this one up, just a bit…
d

David Beck

10/19/2022, 5:34 PM
@Chris Guidry that is my current workaround haha
🤔 1
z

Zanie

10/19/2022, 5:35 PM
Well here’s the simplest fix… https://github.com/PrefectHQ/prefect/pull/7244 I think a validator that peeks at the manifest makes sense but I’m a little worried about the complexity that introduces.
c

Chris Guidry

10/19/2022, 5:36 PM
SMH Michael has the fix faster than I’m writing the issue
🤣 1
z

Zanie

10/19/2022, 5:36 PM
😛
c
z

Zanie

10/19/2022, 5:37 PM
I think a root validator makes the most sense which will definitely take longer
c

Chris Guidry

10/19/2022, 5:37 PM
Updated your PR desc too
z

Zanie

10/19/2022, 5:37 PM
👍 thanks
c

Chris Guidry

10/19/2022, 5:38 PM
I actually like this, it’s simple, and we know for sure that no one could be relying on a
None
value today, so the backwards-compatibility story is solid
d

David Beck

10/19/2022, 5:53 PM
This is great! Seems like this fix will be available soon with the next release :magic_wand:
z

Zanie

10/19/2022, 5:54 PM
Probably available tomorrow 😉
🙌 1
d

David Beck

10/19/2022, 5:55 PM
Yeah I looked it over, and I think that is the correct behavior everyone would want
z

Zanie

10/19/2022, 5:56 PM
Yeah I don’t want to make people have to pass
None
to override our default when we can just peek ourselves.
👍 1