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

Fix setting task run mode in Edit Runtime #65

Open
wants to merge 2 commits into
base: feature.skip_mode
Choose a base branch
from

Conversation

MetRonnie
Copy link

@MetRonnie MetRonnie commented Nov 12, 2024

PR against cylc#6039

Comment on lines +866 to +868
# We have to manually lowercase the run_mode field because we don't define
# a proper schema for BroadcastSetting (it's just GenericScalar) so
# Graphene has no way to know that it should be a TaskRunMode enum.
Copy link
Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Is the graphene.Enum back to front per-chance?

Copy link
Author

Choose a reason for hiding this comment

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

No, the Enum works correctly as far as creating the dropdown and sending the result back

image

This is because the Edit Runtime form is built from the result of querying the runtime of the task proxy:

class Runtime(ObjectType):
    ...
    run_mode = TaskRunMode(default_value=TaskRunMode.Live.name)

The problem is that in the mutation it sends back, there is no longer any association with this Enum, all you get is List(BroadcastSetting) which is the same as List(GenericScalar)

class Broadcast(Mutation):
    ...
    class Arguments:
        ...
        settings = graphene.List(
            BroadcastSetting,
            description=sstrip('''
                The `[runtime]` configuration to override with this broadcast
                (e.g. `script = true` or `[environment]ANSWER=42`).
            '''),

Choose a reason for hiding this comment

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

(sorry, was slow on the uptake there)

@MetRonnie MetRonnie marked this pull request as draft November 12, 2024 16:23
@MetRonnie MetRonnie marked this pull request as ready for review November 12, 2024 19:13
@MetRonnie
Copy link
Author

MetRonnie commented Nov 12, 2024

I think this is actually easier than making the Enum name lowercase and sentence-casing it in the UI; the form generator code in the UI is difficult to work with and could do with a refactor at some point

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.

2 participants