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

Enhancement: Disable Deployments #15129

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

dylanbhughes
Copy link
Contributor

@dylanbhughes dylanbhughes commented Aug 29, 2024

Closes #14529

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

@dylanbhughes dylanbhughes changed the title Enhancement/#14529/disabled deployments8 Enhancement: Disabled Deployments Aug 29, 2024
@github-actions github-actions bot added 3.x api Related the Prefect REST API enhancement An improvement of an existing feature feature A new feature labels Aug 29, 2024
Copy link

codspeed-hq bot commented Aug 29, 2024

CodSpeed Performance Report

Merging #15129 will not alter performance

Comparing enhancement/#14529/disabled-deployments (1b405b1) with main (6dd34dc)

Summary

✅ 3 untouched benchmarks

@dylanbhughes dylanbhughes changed the title Enhancement: Disabled Deployments Enhancement: Disable Deployments Aug 29, 2024
@dylanbhughes dylanbhughes force-pushed the enhancement/#14529/disabled-deployments branch from a093de5 to 219536d Compare August 29, 2024 13:48
@dylanbhughes dylanbhughes force-pushed the enhancement/#14529/disabled-deployments branch from 219536d to 7a0b4d6 Compare August 29, 2024 15:25
Copy link
Contributor

@znicholasbrown znicholasbrown left a comment

Choose a reason for hiding this comment

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

UI side of this looks good, just a minor nit

ui/src/pages/Deployment.vue Outdated Show resolved Hide resolved
ui/src/pages/Deployments.vue Outdated Show resolved Hide resolved
@aaazzam
Copy link
Collaborator

aaazzam commented Sep 5, 2024

Hey @dylanbhughes!

Excited to get this toggle back on the deployment list view. There's a pretty gnarly bug / regression in #14973 which will have some non-breaking but otherwise nontrivial implications for how schedules are created. For the 3.0 branch, I consider work on that issue as blocking on getting this merged 😞, so hang tight. The exact spelling of the bugfix isn't settled yet, but it may require changes to the backend design here (double 😞). No action needed on your part, but we should pause on this for now.

Copy link
Contributor

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

@dylanbhughes
Copy link
Contributor Author

comment

@dylanbhughes
Copy link
Contributor Author

comment

Copy link
Contributor

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

@dylanbhughes
Copy link
Contributor Author

comment

@dylanbhughes
Copy link
Contributor Author

dylanbhughes commented Oct 23, 2024

Hey Team 👋 wanted to resurface this PR. I'm not 100% sure that this issue is really blocking. It doesn't seem like we intend to change the schedule reactivation behavior when a flow is deployed.

Since we're seeing some user interest in the feature, I wanted to see if we could reignite discussion on the disabling deployments feature and see if there's a path to getting this merged.

cc @aaazzam @zzstoatzz @cicdw @desertaxle

@desertaxle
Copy link
Member

@dylanbhughes I think the issue that @aaazzam called out is tangentially related since changing the disabled/enabled state of a deployment via any of our SDK methods could change the schedule state unexpectedly. That's an issue with the SDK and not this work, so I think we could find a way to implement this without first resolving that issue.

Here's a potential way to add this feature while steering clear of our current footguns: this feature starts as an API/UI feature only. This will allow users to disable deployments from the UI where it's most likely to be used, and we can add this feature to the SDK once we've improved our deployment update story.

Happy to hear pushback or other ideas!

@dylanbhughes
Copy link
Contributor Author

I think that sounds great! Disabling or enabling a deployment feels like it should be a pretty explicit action and probably shouldn't occur as a side effect of another action anyway. I'll clean this PR up and do some testing -- hoping to have something by EOW/early next week.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment.

@dylanbhughes
Copy link
Contributor Author

comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x api Related the Prefect REST API enhancement An improvement of an existing feature feature A new feature migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "disabled" property to deployments
4 participants