-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Remove loader support for nodeup tasks not used in models #9170
Conversation
I'm surprised this one isn't used; I'm going to dig into why it isn't / where it came from. On removing the serialization, I do think it's a good idea to clean up the custom deserialization. I do have a longer term idea that we should put less logic into nodeup, and pass it more of these tasks (as I think the logic in nodeup is much more opaque, harder to understand, harder to change). But I don't think that is incompatible with removing the "legacy" deserialization. (And we should just rely on yaml/json deserialization if we start passing raw tasks, IMO) |
Yup, you're right ... unused legacy code. Thanks @johngmyers! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I find tasks in Go code to be easier to understand and change than those in YAML. My IDE can cross-reference the Go code. |
/retest |
1 similar comment
/retest |
/kind cleanup