Skip to content
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

Add support for KubeVirt #1608

Merged
merged 10 commits into from
Sep 27, 2019
Merged

Add support for KubeVirt #1608

merged 10 commits into from
Sep 27, 2019

Conversation

maciaszczykm
Copy link
Contributor

@maciaszczykm maciaszczykm commented Sep 25, 2019

What this PR does / why we need it: Implements UI for API that got added in https://github.com/kubermatic/kubermatic/pull/3942/files#diff-adda64b1de59f751a6e3a59b645067a1R70.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #1576.

Special notes for your reviewer: Missing:

  • validation
  • which fields are required, what about defaulting
  • update wizard summary

Zrzut ekranu 2019-09-25 o 13 46 02

Release note:

Added support for KubeVirt provider.

@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2019
@codecov
Copy link

codecov bot commented Sep 25, 2019

Codecov Report

Merging #1608 into master will decrease coverage by 0.18%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1608      +/-   ##
==========================================
- Coverage   71.92%   71.73%   -0.19%     
==========================================
  Files         209      211       +2     
  Lines        6731     6792      +61     
  Branches      666      673       +7     
==========================================
+ Hits         4841     4872      +31     
- Misses       1632     1662      +30     
  Partials      258      258
Impacted Files Coverage Δ
src/app/testing/fake-data/cluster.fake.ts 95.83% <ø> (ø) ⬆️
src/app/testing/fake-data/node.fake.ts 100% <ø> (ø) ⬆️
src/app/shared/entity/NodeEntity.ts 72.91% <ø> (ø) ⬆️
...sting/fake-data/clusterWithMachineNetworks.fake.ts 100% <ø> (ø) ⬆️
src/app/testing/fake-data/cloud-spec.fake.ts 100% <ø> (ø) ⬆️
src/app/shared/entity/DatacenterEntity.ts 88.88% <ø> (ø) ⬆️
src/app/shared/entity/ClusterEntity.ts 62.16% <0%> (-1.73%) ⬇️
...rc/app/shared/utils/cluster-utils/cluster-utils.ts 84.21% <0%> (-4.68%) ⬇️
src/app/shared/model/NodeProviderConstants.ts 100% <100%> (ø) ⬆️
src/polyfills.ts 100% <100%> (ø) ⬆️
... and 5 more

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 a19b9ea...0079dd1. Read the comment docs.

@kubermatic-bot kubermatic-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2019
Add animations for Angular Material with HammerJS import
@kubermatic-bot kubermatic-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2019
@@ -68,6 +69,7 @@ const appearance: MatFormFieldDefaultOptions = {
provide: MAT_FORM_FIELD_DEFAULT_OPTIONS,
useValue: appearance,
},
{provide: HAMMER_GESTURE_CONFIG, useClass: GestureConfig},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kgroschoff @floreks Interesting thing: Sliders and some of Angular Material animations will only work with this in the code. Otherwise components show up, but you cannot smootly use slider for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@floreks We should use it in k8s/dashboard too.

@maciaszczykm maciaszczykm changed the title [WIP] Add support for KubeVirt Add support for KubeVirt Sep 26, 2019
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2019
@kubermatic-bot kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2019
@maciaszczykm
Copy link
Contributor Author

@maciaszczykm
Copy link
Contributor Author

Kubeconfig is in our Vault.

@kgroschoff
Copy link
Contributor

kgroschoff commented Sep 26, 2019

I already have a few "small" remarks from testing it locally:

  • Could you remove the extended options bar, as there are no optional fields?
  • In Summary view, Provider headline could be removed if there is no preset and we don't want to display the kubeconfig.
  • Edit Provider Settings will cause error in browser dev console zone.js:3331 GET http://localhost:8000/api/v1/projects/jphvfsp68c/dc/europe-west3-c/clusters/52v8k7lwfn/health 404 (Not Found) - could you have a look, please? Edit Provider Settings is empty and I didn't get an error, but still, could you have a look please? 😅
  • Edit Cluster Name is not possible (we could probably fix this issue together with Renaming a cluster will throw errors #1583 )

Apart from those issues, everything seems to work fine (at least for me)

And one general question that came to my mind:
Should we stick with 1024M or 10Gi for input fields or do we want to enter e.g. Memory in GB (or whatever fits best)?

Will have a look into code now :)

Copy link
Contributor

@kgroschoff kgroschoff left a comment

Choose a reason for hiding this comment

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

looks good to me apart from the above mentioned comments, will let @floreks add the final lgtm 😄

hetzner: null,
vsphere: null,
azure: null,
gcp: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@Input() cloudSpec: CloudSpec;
@Input() nodeData: NodeData;
@Input() clusterId: string;
formGroup: FormGroup;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important at all, but: I think we're more or less using form everywhere else. Maybe we want to stick to one pattern for all? I personally don't care if form or formGroup 😅

@floreks
Copy link
Contributor

floreks commented Sep 27, 2019

/lgtm
/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 88ad25aa82029258165215ecc17396ffcc7804f3

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, maciaszczykm

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:
  • OWNERS [floreks,maciaszczykm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@maciaszczykm
Copy link
Contributor Author

/retest

1 similar comment
@maciaszczykm
Copy link
Contributor Author

/retest

@kubermatic-bot kubermatic-bot merged commit a48d322 into master Sep 27, 2019
@kubermatic-bot kubermatic-bot deleted the feature/kubevirt-support branch September 27, 2019 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add kubevirt support to the Dashboard
4 participants