-
Notifications
You must be signed in to change notification settings - Fork 16
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: Remove old flow run infra override access checks #2417
Conversation
@zhen0 Yes, this looks right, we should have removed this alongside the other PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an unused can
import in the automation actions run deployment input file now, otheriwse this LGTM!
nvm my guess above was wrong. I don't see any server-side support for job variables at the schedule level so we do still want to keep that UI behavior turned off. altho if we decided to nix that medium-to-long-term then feature-flagged UI behavior there is confusing. |
@@ -124,7 +124,7 @@ | |||
const workQueueName = ref<string | null>(props.deployment.workQueueName) | |||
const retries = ref<number | null>(null) | |||
const retryDelay = ref<number | null>(null) | |||
const jobVariables = ref<string | undefined>(can.access.flowRunInfraOverrides ? '{}' : undefined) | |||
const jobVariables = ref<string | undefined>('{}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like a bunch of handling could get simplified now by removing this | undefined
in places....
but this should all work as-is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks the API itself calls this field optional so technically the typing is correct but it'd certainly make everything simpler if we stuck to a single type especially if we're defaulting to a defined value.
probably not worth the headache atm tho tbh
PrefectHQ/prefect#12742 removed experimental flags for flow run infra overrides. However the UI still contains access checks meaning that users in OSS can not access or view the flow run infra overrides section of the custom run form (and elsewhere).
This PR removes the access checks. Accompanying PR in PrefectHQ/prefect#13401