feat!: move workspace renames behind flag, disable by default #11189
Conversation
|
Will review shortly. |
|
Re the expiration going into the DB vs. hard-coded, I intentionally envisioned it as hard-coded so it can be easily changed or removed in response to user feedback. |
| <FormFooter | ||
| onCancel={onCancel} | ||
| isLoading={form.isSubmitting} | ||
| submitDisabled={!templatePoliciesEnabled && !workspace.allow_renames} |
There was a problem hiding this comment.
As a general rule, whenever there's a disabled field in the frontend there must be some explanation about why that field is disabled and what (if anything) the user can do to enable it.
There was a problem hiding this comment.
Shouldn't we remove/hide this entire form if allow renames is false? This should only be disabled if allow renames = true and expired.
Otherwise users will be unhappy about seeing a feature that can't be enabled (after the date).
There was a problem hiding this comment.
I'm good with removing the form entirely as well.
There was a problem hiding this comment.
I'd like to keep the structure of the form, since it can optionally be expanded to include template policies if those are enabled. Also I don't love the idea of conditionally having items in a sidebar, which would be the case here if we removed it from the sidebar (because there's nothing to show).
When you mean remove the form entirely, do you mean just the form footer buttons, or actually the whole page? Example of the former.
There was a problem hiding this comment.
For comparison with the template cpolicies enabled:
There was a problem hiding this comment.
Something like
const formEnabled = (templatePoliciesEnabled && !workspace.template_require_active_version)
|| workspace.allow_renames
There was a problem hiding this comment.
I took the liberty of updating the title and marking this a breaking change, since it sounded too much like we're adding a new feature instead of removing one.
The breaking change will also help inform our users that this is going away in the future.
That said, I'm very conflicted about this PR for two reasons:
- We're removing a feature, the general rule is that once it's in you can't remove it because everybody is already relying on it. Do we have data to suggest nobody uses it?
- Timed expiry feels user-hostile, compared to deprecation and explicit removal
Generally when I use software I like the comfort of knowing that behaviors in my applications don't change when I don't update.
If it's a big problem that we forget to remove deprecations from the code-base, can't we make some changes and just lint it instead?
| <FormFooter | ||
| onCancel={onCancel} | ||
| isLoading={form.isSubmitting} | ||
| submitDisabled={!templatePoliciesEnabled && !workspace.allow_renames} |
There was a problem hiding this comment.
Shouldn't we remove/hide this entire form if allow renames is false? This should only be disabled if allow renames = true and expired.
Otherwise users will be unhappy about seeing a feature that can't be enabled (after the date).
|
@mafredri I can agree with some of your points, I do think the timed expiration feels a little hostile because it is not in the usual upgrade path admins will be expecting.
This is a good point and one I didn't fully consider. If we are only checking at start, the response could be different depending on when the instance started in an HA setting. Also if the app stays running without restart for a long time, it can go way past the deadline and continue allowing until it does restart.
I considered this but didn't want to bleed this logic into the upper components, and assumed we will add more settings in the future here so I think it's OK to have a disabled form that users can see. There are other tabs that should be viewed as well. @ammario Because of the issue with both the golden files breaking CI if we auto-hide the flag at a set date + the odd startup behavior, I think the deadline behavior might not be the best experience for us and the customer. Do you want to continue with this behavior - do the auto-hide behavior (and opt to break CI in the future), revert this to a basic flag we remove later, or something else? |
|
I'm thinking the CI breaking at a certain date is a good failsafe. My p1 is solving for us keeping around deprecated options for too long, p3 is enforcing customers migrate at a date. |
How will we enforce that it remains a fail-safe? I see it as a very high risk of this getting in the way of multiple PRs at a certain date, I for one don't want to be dealing with sudden CI breakage. |
| <FormFooter | ||
| onCancel={onCancel} | ||
| isLoading={form.isSubmitting} | ||
| submitDisabled={!templatePoliciesEnabled && !workspace.allow_renames} |
There was a problem hiding this comment.
I'm good with removing the form entirely as well.
6286cf2 to
db34075
Compare
There was a problem hiding this comment.
BE LGTM 👍🏻, only thing that raised a flag for me was the remov 10000 al of the form footer in some cases for the FE, but I'll leave that for FE people determine.
I'm still a bit disappointed we're removing the feature, but this approach is at least acceptable. 😄 Thanks for making the changes @f0ssel.
| helperText={ | ||
| !workspace.allow_renames && | ||
| "Renaming your workspace can be destructive and has not been enabled for this deployment." | ||
| } |
There was a problem hiding this comment.
We have an alert below that relies on slightly different logic but has similar messaging. It would be nice to consolidate and just use helper text IMO. You could even conditionally change the color of the text so that it assumes a warning hue if workspace.allow_renames is true.
The class to target would be .MuiFormHelperText-root
| @@ -54,16 +58,20 @@ export const WorkspaceSettingsForm: FC<{ | |||
| <HorizontalForm onSubmit={form.handleSubmit} data-testid="form"> | |||
| <FormSection | |||
| title="Workspace Name" | |||
| description="Update the name of your workspace." | |||
| description="The name of your workspace." | |||
There was a problem hiding this comment.
I might keep the old language here. The user already knows this is an input. Also, the other field (and any other fields we add in the future) are going to use imperative descriptions.
|
Everything works as intended! From a FE perspective, I'm not wild about having a disabled form, but I think the design required here is outside of this ticket's scope. We should probably make a ticket for a more general redesign of these workspace forms. If a template doesn't have parameters and also doesn't have this flag enabled, we basically have two pages of empty space. Also, could we please add 1) a storybook for the disabled input to |
Closes #10922
Considerations