Skip to content
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

feat!: Add an 'operation' argument to the job bundle export callback. #92

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

rmv
Copy link
Contributor

@rmv rmv commented Oct 26, 2023

What was the problem/requirement? (What/Why)

When submitting a job to Deadline Cloud, it should be possible to run on the DCC certain sanity checks that are not required when exporting a bundle. For instance, when submitting, it makes sense to check if the scene file has become stale, and alert the user that the file should be saved. But when exporting a bundle that check makes no sense.

Currently there's no way for the DCC submitter to determine if the create_job_bundle_callback is called because of an export or a submission.

What was the solution? (How)

This patch adds a new argument to the callback with an operation enum so that client code can determine if we are exporting or submitting. It also refactors the callback calls to avoid repeating the code related to backwards compatibility.

What is the impact of this change?

This is backwards compatible with submitters that don't expect that argument. It should make it possible for submitters to cancel a submission early given DCC specific tests while leaving export functionality intact.

How was this change tested?

With the Maya submitter, using a callback signature with and without the new argument.

Was this change documented?

No.

Is this a breaking change?

No.

@rmv rmv requested a review from a team as a code owner October 26, 2023 02:59
# ---- Types to export


class CallbackOperation(str, Enum):
Copy link
Contributor

@mwiebe mwiebe Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Callback" doesn't feel like the right adjective here. Yes, it's used in a callback, but "export" and "submission" aren't about the general idea of callbacks, they're about creating a job bundle.

Maybe JobBundlePurpose or JobBundleReason better characterizes this?

@@ -322,6 +323,54 @@ def on_settings_button_clicked(self):
if DeadlineConfigDialog.configure_settings(parent=self):
self.refresh_deadline_settings()

def _on_create_job_bundle_callback(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the plan to remove this? I worry a bit that code like this would just keep getting added, instead of being tidied up when things are switched to the new option.

What if we first modified the downstream consumers to accept the operation (or maybe purpose or reason to match the enum name). The changes there require a lot less code to be backwards compatible. Once they're all released, then update the code here by directly modifying the call instead of adding this shim.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good!

@rmv
Copy link
Contributor Author

rmv commented Oct 26, 2023

Split the type definition part into #94
(a pre requisite for the changes required in the submitters)

Signed-off-by: Ramon Montoya Vozmediano <1171039+rmv@users.noreply.github.com>
@rmv rmv changed the title feat: Add an 'operation' argument to the job bundle export callback. feat!: Add an 'operation' argument to the job bundle export callback. Nov 7, 2023
@rmv rmv merged commit bc60165 into mainline Nov 8, 2023
18 checks passed
@rmv rmv deleted the rmvh/add_callback_reason branch November 8, 2023 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants