-
Notifications
You must be signed in to change notification settings - Fork 887
fix: prompt when parameter options are incompatible #9247
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
Conversation
@@ -1243,6 +1242,35 @@ const getMissingParameters = ( | |||
missingParameters.push(parameter) | |||
} | |||
|
|||
// Check if parameter "options" changed and we can't use old build parameters. |
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.
@BrunoQuaresma I may need your frontend wisdom. I battling the following edge case here:
- We have a
foobar
string parameter with options:aaa
,bbb
(default),ccc
, and the workspace built on it. The workspace usesbbb
option. - Change the parameter option
bbb
to something else, let's sayddd
. - Click "Update".
I can see the UpdateParametersDialog, but when I click the Update
button, it gets back to it. If I pick something else, and then click back to the default, it works fine.
I looked deeper, and it seems that Formik doesn't set the internal form.values
, even though initialValues
are passed. I'm not sure how I can proceed here.
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.
Apparently, I got it fixes with enableReinitialize
. Thanks for the suggestion!
@@ -981,6 +981,8 @@ func (p *prettyErrorFormatter) format(err error) { | |||
msg = sdkError.Message | |||
if sdkError.Helper != "" { | |||
msg = msg + "\n" + sdkError.Helper | |||
} else if sdkError.Detail != "" { |
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.
for reviewers:
Message: "Unable to validate parameter X"
Detail: "parameter value must match one of options"
^ this should be consistent with site UI error
@@ -69,7 +69,6 @@ const ( | |||
type JobErrorCode string | |||
|
|||
const ( | |||
MissingTemplateParameter JobErrorCode = "MISSING_TEMPLATE_PARAMETER" |
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.
for reviewers: used by old legacy parameters (do not exist anymore)
@@ -1243,6 +1242,35 @@ const getMissingParameters = ( | |||
missingParameters.push(parameter) | |||
} | |||
|
|||
// Check if parameter "options" changed and we can't use old build parameters. |
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.
Apparently, I got it fixes with enableReinitialize
. Thanks for the suggestion!
<-doneChan | ||
}) | ||
|
||
t.Run("ParameterOptionChanged", func(t *testing.T) { |
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.
Can you add a test case where the chosen template parameter disappears completely?
e.g. before: { 1st, 2nd, 3rd }, after: { 1st, 3rd, 4th }
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.
Added!
Related: #9163
This PR modifies CLI and site to prompt user for correct parameter options if they are incompatible now (new template version submitted).