Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

PlanDetails: Refactor using IPlan object #620

Merged
merged 1 commit into from Jun 3, 2021
Merged

PlanDetails: Refactor using IPlan object #620

merged 1 commit into from Jun 3, 2021

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jun 3, 2021

Using generatePlan and generateMappings to create a non resilient IPlan object to feed PlandDetails instead of fields.

@gildub gildub requested a review from a team June 3, 2021 07:20
@konveyor-preview-bot
Copy link

🚀 Deployed Preview: http://konveyor-forklift-ui-pr-620-preview.surge.sh

Compare with current main branch: http://konveyor-forklift-ui-preview.surge.sh

@codecov
Copy link

codecov bot commented Jun 3, 2021

Codecov Report

Merging #620 (e5ca157) into main (2e64d3b) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #620      +/-   ##
==========================================
+ Coverage   59.23%   59.29%   +0.06%     
==========================================
  Files         144      144              
  Lines        4776     4771       -5     
  Branches     1312     1313       +1     
==========================================
  Hits         2829     2829              
+ Misses       1909     1904       -5     
  Partials       38       38              
Impacted Files Coverage Δ
src/app/Plans/components/PlanDetails.tsx 83.72% <100.00%> (-1.70%) ⬇️
src/app/Plans/components/PlanDetailsModal.tsx 21.42% <100.00%> (+0.49%) ⬆️
src/app/Plans/components/Wizard/Review.tsx 96.87% <100.00%> (+0.10%) ⬆️
src/app/Plans/components/Wizard/helpers.tsx 92.01% <0.00%> (+1.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e64d3b...e5ca157. Read the comment docs.

@gildub gildub requested a review from mturley June 3, 2021 07:25
@gildub gildub changed the title Refactors PlanDetails to use IPlan object PlanDetails: Refactor using IPlan object Jun 3, 2021
@gildub gildub changed the title PlanDetails: Refactor using IPlan object [WIP] PlanDetails: Refactor using IPlan object Jun 3, 2021
@gildub gildub changed the title [WIP] PlanDetails: Refactor using IPlan object PlanDetails: Refactor using IPlan object Jun 3, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comment to address later.

const plan: IPlan = generatePlan(
forms,
{
name: (networkMapping?.metadata as IMetaObjectMeta).name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generateMappings won't produce a mapping with a metadata.name field if the wizard has no existing mapping being edited or no new mapping name being entered (instead it will use the generateName field). this as IMetaObjectMeta assertion here prevents TS from catching this. So this name field will be undefined in that case...

However, that doesn't really matter here because the plan.spec.map that is generated here isn't being used at all. So honestly we could just pass { name: '', namespace: '' } for these mapping refs instead. If so we should probably leave a comment explaining that real mappings are not necessary for this ephemeral plan object.

I'm going to merge anyway so I can prevent merge conflicts with #597, but we should probably clean this up later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mturley, noted, I've added #627

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mturley mturley merged commit 2658663 into kubev2v:main Jun 3, 2021
@gildub gildub deleted the plandetails-iplan branch June 7, 2021 10:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants