Add swagger ui to render api docs #1418

bashofmann · 2019-07-25T14:28:30Z

What this PR does / why we need it:
Add swagger ui to render api docs.

This depends on kubermatic/kubermatic#3890.

What we are not sure yet: Where would be the best place to add the link to it, Footer or SideNav?

Release note:

Add Swagger UI

kubermatic-bot · 2019-07-25T14:28:38Z

Hi @bashofmann. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

package.json

src/app/pages/api-docs/api-docs.component.ts

src/polyfills.ts

floreks · 2019-07-29T07:40:54Z

The placement IMHO is fine. The other option would be to move it to the left and make it powered by Loodse - API - version. I'd also maybe shorten it from API Docs to just API.

I'd also hide the sidebar when viewing the API. We can make kubermatic logo a link to main projects view.

@cschieder @maciaszczykm @kgroschoff WDYT?

kgroschoff · 2019-07-29T07:52:06Z

Absolutely agree with @floreks.
Additionally I think it would be great if you could configure if you want to display the link or not. (like Demo System or Terms of Service)

maciaszczykm · 2019-07-29T08:08:45Z

+1 to API instead of API Docs.

bashofmann · 2019-07-30T08:12:11Z

I addressed the comments above, renamed the link to "API", moved it to the left of the footer, disabled the sidenav and made the link configurable.

maciaszczykm · 2019-07-30T08:12:21Z

/test all

bashofmann · 2019-07-30T08:18:23Z

When I add the routerlink to the logo in the NavigationComponent, I get the following error running the tests, no idea how to fix this one though:

HeadlessChrome 76.0.3809 (Mac OS X 10.14.6) NavigationComponent should tell Router to navigate when user logout FAILED
	TypeError: Cannot read property 'root' of undefined
	error properties: Object({ ngDebugContext: DebugContext_({ view: Object({ def: Object({ factory: Function, nodeFlags: 51101697, rootNodeFlags: 1, nodeMatchedQueries: 6, flags: 0, nodes: [ Object({ nodeIndex: 0, parent: null, renderParent: null, bindingIndex: 0, outputIndex: 0, checkIndex: 0, flags: 1, childFlags: 51101697, directChildFlags: 33554433, childMatchedQueries: 6, matchedQueries: Object({  }), matchedQueryIds: 0, references: Object({  }), ngContentIndex: null, childCount: 9, bindings: [  ], bindingFlags: 0, outputs: [  ], element: Object({ ns: '', name: 'header', attrs: [  ], template: null, componentProvider: null, componentView: null, componentRendererType: null, publicProviders: null({  }), allProviders: null({  }), handleEvent: Function }), provider: null, text: null, query: null, ngContent: null }), Object({ nodeIndex: 1, parent: Object({ nodeIndex: 0, parent: null, renderParent: null, bindingIndex: 0, outputIndex: 0, checkIndex: 0, flags: 1, childFlags: 51101697, directChildFlags: 33 ...
	    at <Jasmine>
	    at rootRoute (http://localhost:9877/node_modules/@angular/router/fesm5/router.js:5628:1)
	    at _callFactory (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:20959:1)
	    at _createProviderInstance (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:20915:1)
	    at resolveNgModuleDep (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:20876:1)
	    at NgModuleRef_.get (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:21584:1)
	    at resolveDep (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:21955:1)
	    at createClass (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:21831:1)
	    at createDirectiveInstance (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:21706:1)
	    at createViewNodes (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:29881:1)
	    at callViewAction (http://localhost:9877/node_modules/@angular/core/fesm5/core.js:30197:1)
	TypeError: Cannot read property 'debugElement' of undefined
	    at <Jasmine>
	    at UserContext.<anonymous> (http://localhost:9877/src/app/core/components/navigation/navigation.component.spec.ts:61:30)
	    at TestBedViewEngine.execute (http://localhost:9877/node_modules/@angular/core/fesm5/testing.js:2430:1)
	    at UserContext.<anonymous> (http://localhost:9877/node_modules/@angular/core/fesm5/testing.js:2598:29)
	    at ZoneDelegate../node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (http://localhost:9877/node_modules/zone.js/dist/zone.js:391:1)
	    at ProxyZoneSpec.onInvoke (http://localhost:9877/node_modules/zone.js/dist/proxy.js:129:1)
	    at ZoneDelegate../node_modules/zone.js/dist/zone.js.ZoneDelegate.invoke (http://localhost:9877/node_modules/zone.js/dist/zone.js:390:1)
	    at Zone../node_modules/zone.js/dist/zone.js.Zone.run (http://localhost:9877/node_modules/zone.js/dist/zone.js:150:1)
	    at runInTestZone (http://localhost:9877/node_modules/zone.js/dist/jasmine-patch.js:177:1)
	    at UserContext.<anonymous> (http://localhost:9877/node_modules/zone.js/dist/jasmine-patch.js:192:1)
	    at <Jasmine>

bashofmann · 2019-07-30T08:21:20Z

When I add the routerlink to the logo in the NavigationComponent, I get the following error running the tests, no idea how to fix this one though

I think I found it :).

floreks · 2019-07-30T08:46:22Z

Can you regenerate package-lock.json? Almost 7k lines feels way too much for only 2 new dependencies.

src/app/core/components/footer/footer.component.html

floreks · 2019-07-30T08:48:51Z

src/app/core/components/footer/footer.component.html

+       *ngIf="showApiDocs()">
+    <div class="km-separator">–</div>
+    <a routerLink="api"
+       matTooltip="API">


Isn't tooltip with the same message redundant?

@cschieder @maciaszczykm should we elaborate tooltip message here or remove it completely?

I'd be fine with both.

bashofmann · 2019-07-30T09:21:20Z

Can you regenerate package-lock.json? Almost 7k lines feels way too much for only 2 new dependencies.

I took the package-lock.json from master, ran npm install swagger-ui buffer. The diff is still huge. Any idea what I'm doing wrong?

❯ node -v
v12.6.0

❯ npm -v
6.10.2

floreks · 2019-07-30T09:39:48Z

To regenerate:

Remove node_modules
Remove package-lock.json
Run npm i

This provides the best result. ~500 lines are reasonable as it also adds the whole dependency tree of added dep.

bashofmann · 2019-07-30T09:45:24Z

That seems to have made the diff even larger for me :/

floreks · 2019-07-30T09:47:27Z

😂

Looks like it have just swapped order of dependencies. It does that sometimes, but if you have followed my steps then it's fine. It's better to regenerate it from scratch after adding dependency. Are there any audit issues reported (npm audit)?

bashofmann · 2019-07-30T09:49:15Z

npm audit

                       === npm audit security report ===

# Run  npm install --save-dev cypress@3.4.1  to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ cypress [dev]                                                │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ cypress > lodash                                             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/1065                            │
└───────────────┴──────────────────────────────────────────────────────────────┘


┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ underscore.string                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.3.5                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ swagger-ui                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ swagger-ui > remarkable > argparse > underscore.string       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/745                             │
└───────────────┴──────────────────────────────────────────────────────────────┘
found 2 vulnerabilities (1 moderate, 1 high) in 20601 scanned packages
  run `npm audit fix` to fix 1 of them.
  1 vulnerability requires manual review. See the full report for details.

bashofmann · 2019-07-30T09:51:34Z

Should I update cypress in this PR?

bashofmann · 2019-07-30T11:23:14Z

The other audit issue has been fixed in remarkable, but not released yet: jonschlinkert/remarkable#357.

Fortunately it does not seem to be a problem in swagger: swagger-api/swagger-ui#5152.

bashofmann · 2019-07-30T15:00:18Z

Why does adding routerLink="projects" to the NavigationComponent introduce an API call that needs mocking?

Or do you mean that the route must not start with /api. The problem there is that if you directly go to the URL (or refresh the page) the Ingress Controller directly directs the request to kubermatic-api and not to the dasbhoard (/~https://github.com/kubermatic/kubermatic/blob/master/config/kubermatic/templates/ingress.yaml#L19)

floreks · 2019-07-30T15:19:05Z

Ok, so the solution is quite simple. I've had to look into the code. Simply remove {provide: Router, useClass: RouterStub}, from the spec (and your changes). It should work.

floreks · 2019-07-31T07:23:26Z

/test all

codecov · 2019-07-31T07:32:53Z

Codecov Report

Merging #1418 into master will decrease coverage by 0.21%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #1418      +/-   ##
==========================================
- Coverage    73.4%   73.19%   -0.22%     
==========================================
  Files         205      206       +1     
  Lines        6288     6316      +28     
  Branches      582      591       +9     
==========================================
+ Hits         4616     4623       +7     
- Misses       1428     1452      +24     
+ Partials      244      241       -3

Impacted Files	Coverage Δ
src/app/kubermatic.component.ts	`54.16% <0%> (ø)`	⬆️
src/polyfills.ts	`100% <100%> (ø)`	⬆️
src/app/core/components/footer/footer.component.ts	`95.23% <100%> (+0.5%)`	⬆️
src/app/core/services/api/api.service.ts	`36.92% <33.33%> (+1.49%)`	⬆️
src/app/wizard/summary/summary.component.ts	`57.89% <0%> (-25.83%)`	⬇️
...c/app/shared/components/labels/labels.component.ts	`41.17% <0%> (-23.53%)`	⬇️
.../app/wizard/set-settings/set-settings.component.ts	`71.42% <0%> (-14.29%)`	⬇️
...ngs/custom-credentials/custom-presets.component.ts	`81.08% <0%> (-2.71%)`	⬇️
...r/cluster-details/node-list/node-list.component.ts	`77.89% <0%> (-0.6%)`	⬇️
src/app/sshkey/sshkey.component.ts	`88.23% <0%> (-0.18%)`	⬇️
... and 13 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 09f51a4...4ffb218. Read the comment docs.

floreks · 2019-07-31T07:49:28Z

Please rebase as the base of your branch does not contain some of the e2e fixes.

maciaszczykm · 2019-07-31T07:54:41Z

/test all

* Rename link * Make API link configurable in config * Link header logo to /projects * Inject document instead of using global window * Remove unnecessary polyfill

…ontroller routing /api to the kubermatic-api

bashofmann · 2019-07-31T09:34:02Z

Rebase done

cschieder · 2019-07-31T10:14:21Z

Can we fix the lag on the sidebar animation when you go to the API docs somehow? Also on the API page please add the usual shading to the card

bashofmann · 2019-07-31T12:11:58Z

I added the box-shadow, but I'm not sure how I could improve the sidebar animation, the problem is likely that the rendering of the swagger UI that happens at the same time as the animation introduces this lag.

floreks · 2019-08-05T08:09:30Z

/retest

floreks · 2019-08-05T08:32:06Z

/lgtm
/approve

kubermatic-bot · 2019-08-05T08:32:13Z

LGTM label has been added.

Git tree hash: 826cb5e0208f596c936f815ac8c2f146f9f154c4

kubermatic-bot · 2019-08-05T08:32:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks

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]

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

floreks · 2019-08-05T08:54:57Z

/test pull-dashboard-e2e-test

kubermatic-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 25, 2019

kubermatic-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 25, 2019

bashofmann mentioned this pull request Jul 25, 2019

Create a API page rendering our swagger #1411

Closed