-
Notifications
You must be signed in to change notification settings - Fork 900
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(triggers): do not allow manual definition of docker image in trigger #7561
fix(triggers): do not allow manual definition of docker image in trigger #7561
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.
💯
@@ -447,7 +450,7 @@ export class DockerImageAndTagSelector extends React.Component< | |||
<span className="field"> | |||
<Select | |||
value={defineManually} | |||
disabled={imagesLoading} | |||
disabled={imagesLoading || !allowManualDefinition} |
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.
thoughts on just removing this select field entirely when manual entry is disabled?
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 guess we'd have to figure out what the visual treatment would look like since it lives in that grey header thing...
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.
Yeah, removing the select field entirely was my initial thought as well, followed by the realization that it lives in that header thing and that we'd probably want to convert this entire component to use the formik form fields at some point anyway so might not be worth handling the visual treatment now? But if you think it looks really weird with the select field disabled let me know!
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 you did the right thing — we just didn't account for wanting to omit this selection entirely when it was originally designed. You took this journey of discovery a little before me but now we've all ended up at the same place 🤣
17232e6 fix(triggers): do not allow manual definition of docker image in trigger (spinnaker#7561)
17232e6 fix(triggers): do not allow manual definition of docker image in trigger (spinnaker#7561)
17232e6 fix(triggers): do not allow manual definition of docker image in trigger (spinnaker#7561)
Closes spinnaker/spinnaker#4929
Disallows manual entry of a Docker image when configuring a trigger.