-
Notifications
You must be signed in to change notification settings - Fork 408
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
Allow disconnecting service-connector from stack component #1864
Allow disconnecting service-connector from stack component #1864
Conversation
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.
Great functionality, looking forward to using that 👍
Overall the implementation looks good, however I would prefer if disconnect
was a separate field in the StackComponentUpdateModel
since connector=None
usually just means that this value was not provided and should not be updated. See comment below.
Other than that only minor issues, looks like we can merge it today 🚀
else: | ||
existing_component.connector = None | ||
existing_component.connector_resource_id = None |
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.
Currently, if any value in an update model is None
, it means that this value was not provided and should not be updated. So I think the current design is breaking this convention since connector=None
now disconnects the existing connector and thereby leads to updates.
Instead, I would suggest to add disconnect
as a field of StackComponentUpdateModel
explicitly and then use this here instead:
else: | |
existing_component.connector = None | |
existing_component.connector_resource_id = None | |
elif component_update.disconnect: | |
existing_component.connector = None | |
existing_component.connector_resource_id = None |
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 think it's fine as it is, because this immediately fixes the UI as well. The alternative also requires dashboard changes. If it helps, this is the same approach used for service connector updates: some update fields are "special", in the sense that if set to None or omitted from the update request, they are interpreted as a value removal rather than a value omission.
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.
LGTM aside from the changes that Felix already suggested.
Co-authored-by: Felix Altenberger <felix@zenml.io>
…dd-disconnect-service-connector
Describe changes
I implemented/fixed _ to achieve _.
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes