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 1754478: Migrate TemplateSource component #3154
Bug 1754478: Migrate TemplateSource component #3154
Conversation
@irosenzw: This pull request references Bugzilla bug 1754478, 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. |
Hi @irosenzw. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
should we include any tests ? |
970878b
to
730ba6b
Compare
frontend/packages/kubevirt-plugin/integration-tests/tests/utils/mocks.ts
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/integration-tests/tests/vm.template.source.scenario.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-source.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-source.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/components/vm-templates/vm-template-source.tsx
Outdated
Show resolved
Hide resolved
730ba6b
to
b5bddea
Compare
b5bddea
to
77ec164
Compare
feedfb7
to
3fab0e4
Compare
const isProvisionSource = | ||
dataVolumes && !!_.get(getTemplateProvisionSource(template, dataVolumes), 'type'); | ||
const provisionSource = ProvisionSource.getProvisionSourceDetails(template); | ||
const isProvisionSource = dataVolumes && !!provisionSource.type; |
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.
you can discard the dataVolumes (loading and all) from the vm template details. They are not needed anymore.
@@ -125,7 +125,7 @@ export const VMTemplateDetailsList: React.FC<VMTemplateResourceListProps> = ({ | |||
idValue={prefixedID(id, 'provisioning-source')} | |||
isNotAvail={!isProvisionSource} |
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.
this construct will never show a provision source error
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.
in case we care about them, which I think we should
}; | ||
|
||
export const TemplateSource: React.FC<TemplateSourceProps> = ({ template, detailed = false }) => { | ||
const provisionSource = ProvisionSource.getProvisionSourceDetails(template); |
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.
this call will return errors without a type
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 we should enhance this call for some kind of NA
flag and render Not Available
here
export const resolveURL = ({ urlObj, maxHostnameParts, maxPathnameParts }) => | ||
`${resolveOrigin(urlObj, maxHostnameParts)}${resolvePathname(urlObj, maxPathnameParts)}`; | ||
|
||
export const URLObj: React.FC<URLObjProps> = ({ |
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.
IMO we shouldn't mix components with utils files
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.
otherwise it would look better to git mv
this file instead
return ( | ||
<> | ||
<Type type={type.getValue()} source={source} error={error} /> | ||
<Source type={type.getValue()} source={source} /> |
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.
why not just pass the type reference?
3fab0e4
to
2a2bf0b
Compare
const { type, source, error } = provisionSource; | ||
|
||
if (!provisionSource || !provisionSource.type) { | ||
if (showError) { |
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.
still, 'No bootable device found.'
error is the same as Not available
. I would rather use just the later one to be consistent
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.
Just to be clear, do you want: No bootable device found
or Not available
error message ?
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.
Not available
should be fine
@@ -0,0 +1,254 @@ | |||
export const urlTemplate = { |
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.
IMO, I would rather split these templates into three files and put them to a separate directory
ca57cc5
to
c720ce0
Compare
/retest |
1 similar comment
/retest |
c720ce0
to
3ccb439
Compare
7a8e5a6
to
ac28bf5
Compare
@@ -103,9 +103,13 @@ export class ProvisionSource extends ValueEnum<string> { | |||
return { | |||
error: `Datavolume ${volumeWrapper.getDataVolumeName()} does not have a supported source (${type}).`, | |||
}; | |||
case undefined: |
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 have a special check like this here, but it should go right after the volume lookup.
} | ||
return <div>Invalid source</div>; | ||
} | ||
return <div title={source}>{type.getValue()}</div>; |
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 should try to be consistent - this should also be a tooltip
if (!detailed) { | ||
return ( | ||
<Tooltip position="bottom" enableFlip={false} content={<>{error}</>} exitDelay={0}> | ||
<div>Invalid source</div> |
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.
something like this.. + different ifs
above
<div>Invalid source</div> | |
<div>{error ? 'Invalid Source' : type}</div> |
const sourceElem = | ||
type.getValue() === ProvisionSource.URL.getValue() ? <URLObj urlPath={source} short /> : source; | ||
|
||
return <div>{sourceElem}</div>; |
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.
again consistency - why div here and not for the error?
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.
also, this component should return null if there is no source or an error
} | ||
|
||
const sourceElem = | ||
type.getValue() === ProvisionSource.URL.getValue() ? <URLObj urlPath={source} short /> : source; |
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.
type.getValue() === ProvisionSource.URL.getValue() ? <URLObj urlPath={source} short /> : source; | |
type.getValue() === ProvisionSource.URL.getValue() ? <URLObj urlPath={source} short /> : (source || error); |
15884eb
to
901328b
Compare
} | ||
|
||
const sourceElem = | ||
!type || type.getValue() === ProvisionSource.URL.getValue() ? ( |
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.
!type || type.getValue() === ProvisionSource.URL.getValue() ? ( | |
type && type.getValue() === ProvisionSource.URL.getValue() ? ( |
it is not an URL by default
d4b3c86
to
82e6dd9
Compare
…mponents Signed-off-by: Ido Rosenzwig <irosenzw@redhat.com>
82e6dd9
to
cc96dfe
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irosenzw, suomiy 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 |
/retest |
2 similar comments
/retest |
/retest |
/test e2e-gcp-console |
/retest |
@irosenzw: All pull requests linked via external trackers have merged. Bugzilla bug 1754478 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. |
Migrate TemplateSource component from kubevirt-web-ui-components
Signed-off-by: Ido Rosenzwig irosenzw@redhat.com