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
Bug 1806994: fixes issue with editing knative service created via cli #4464
Bug 1806994: fixes issue with editing knative service created via cli #4464
Conversation
@invincibleJai: This pull request references Bugzilla bug 1806994, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@invincibleJai: This pull request references Bugzilla bug 1806994, which is valid. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/kind bug |
4815184
to
c2fd983
Compare
return { | ||
...deployImageInitialValues, | ||
searchTerm: name, | ||
registry: 'external', |
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 we change this to RegistryType.External
registry: 'external', | |
registry:RegistryType.External, |
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.
Done
const deployImageInitialValues = { | ||
searchTerm: '', | ||
return { | ||
...deployImageInitialValues, | ||
registry: 'internal', |
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.
We could use the enum here RegistryType.Internal.
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.
Done
c2fd983
to
3a0ccca
Compare
@@ -371,6 +375,17 @@ export const getInitialValues = ( | |||
internalImageValues = _.isEmpty(externalImageValues) | |||
? getInternalImageInitialValues(_.get(appResources, 'editAppResource.data')) | |||
: {}; | |||
if ( | |||
_.isEmpty(externalImageValues) && | |||
!_.get(internalImageValues, 'imageStream.tag') && |
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 we optional chaining here instead of get function?
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.
Chaining would look for type
here as above we have {}
in case externalImageValues
is not empty so added get
internalImageValues = _.isEmpty(externalImageValues)
? getInternalImageInitialValues(_.get(appResources, 'editAppResource.data'))
: {};
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
@invincibleJai if we disable edit for knative service created from outside of dev console. Then we don't need this change, correct? (see #4506) If we can't ensure that all edit use cases are covered for ksvc created from cli or elsewhere, then I'm still reluctant to make these changes for 4.4 rather than simply disable the action. |
yeah we don't need this then.
yeah i verified but i think for 4.4 we can be consistent to support Edit flow only for the resources created via DevConsole |
/hold |
@invincibleJai @christianvogt is there a reason we are keeping this open & the ticket still targeted for 4.4? I think we can drop both the PR and the ticket back into the backlog / obsolete it because we're not addressing the edit flow on resources not created by the add flow. |
@andrewballantyne We can drop the 4.4 target on the ticket though. |
Will need a rebase @invincibleJai |
2a3e875
to
27da1a6
Compare
@invincibleJai: This pull request references Bugzilla bug 1806994, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
27da1a6
to
940480f
Compare
@invincibleJai Verified locally, now edit works well for the knative resource created via cli. |
yeah even for other flows on edit Submit button is enabled for edit and for knative on edit we create new |
Verified locally, editing works fine for knative services created via cli and webconsole |
940480f
to
bcdd445
Compare
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
bcdd445
to
367b474
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: invincibleJai, karthikjeeyar, vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@invincibleJai: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@invincibleJai: All pull requests linked via external trackers have merged. Bugzilla bug 1806994 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-3071
Analysis / Root cause:
Edit flow doesn't work in knative service is created via CLI i.e on edit it doesn't show associated image as pre selected
Solution Description:
if associated imageStreams are not found while editing going with value provided by user in yaml for container image
Screen shots / Gifs for design review:
Test Coverage
Browser conformance: