You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/06/21 22:20:03 UTC
[GitHub] [trafficcontrol] shamrickus opened a new pull request, #6914: Add Ability to Extend TPv2
shamrickus opened a new pull request, #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914
<!--
Thank you for contributing! Please be sure to read our contribution guidelines: https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md
If this closes or relates to an existing issue, please reference it using one of the following:
Closes: #ISSUE
Related: #ISSUE
If this PR fixes a security vulnerability, DO NOT submit! Instead, contact
the Apache Traffic Control Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://apache.org/security regarding vulnerability disclosure.
-->
This PR adds a custom module to TPv2 that can be used to add/extend/replace functionality. It also updates Angular to 14 as well as run the 13/14 migrations.
<!-- **^ Add meaningful description above** --><hr/>
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this PR.
Feel free to add the name of a tool or script that is affected but not on the list.
-->
- Traffic Portal v2
## What is the best way to verify this PR?
By default, nothing should've changed. To verify read the docs and make sure they make sense. Follow the docs to enable the custom module, and after logging in navigate to `{url:port}/custom` to see that the bundle is lazy loaded and not included in the default bundle.
<!-- Please include here ALL the steps necessary to test your PR.
If your PR has tests (and most should), provide the steps needed to run the tests.
If not, please provide step-by-step instructions to test the PR manually and explain why your PR does not need tests. -->
<!-- Delete this section if the PR is not a bugfix, or if the bug is only in the master branch.
Examples:
- 5.1.2
- 5.1.3 (RC1)
-->
## PR submission checklist
- [x] This PR has tests <!-- If not, please delete this text and explain why this PR does not need tests. -->
- [x] This PR has documentation <!-- If not, please delete this text and explain why this PR does not need documentation. -->
- [x] This PR has a CHANGELOG.md entry <!-- A fix for a bug from an ATC release, an improvement, or a new feature should have a changelog entry. -->
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904067710
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.component.ts:
##########
@@ -37,17 +37,55 @@ export class TpHeaderComponent implements OnInit {
public hidden = false;
+ // Will try to display each of these navs on the header, space allowing.
+ public horizNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+ // Navs that are not directly displayed on the header.
+ public vertNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+
/**
* Angular lifecycle hook
*/
public ngOnInit(): void {
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core",
+ text: "Home",
+ type: "anchor",
+ }, "home");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/users",
+ text: "Users",
+ type: "anchor",
+ visible: () => this.hasPermission("USER:READ"),
+ }, "Users");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/servers",
+ text: "Servers",
+ type: "anchor",
+ visible: () => this.hasPermission("SERVER:READ"),
+ }, "Servers");
+ this.headerSvc.addVerticalNav({
+ routerLink: "/core/me",
+ text: "Profile",
+ type: "anchor"
+ }, "Profile");
+ this.headerSvc.addVerticalNav({
+ click: async () => this.logout(),
+ text: "Logout",
+ type: "anchor"
+ }, "Logout");
+
this.headerSvc.headerTitle.subscribe(title => {
this.title = title;
});
-
this.headerSvc.headerHidden.subscribe(hidden => {
this.hidden = hidden;
});
+ this.headerSvc.horizontalNavsUpdated.subscribe(navs => {
+ this.horizNavs = navs;
+ });
+ this.headerSvc.verticalNavsUpdated.subscribe(navs => {
+ this.vertNavs = navs;
+ });
Review Comment:
Will do. For the record, It only redid the inserts on the initial creation of the component, which should only happen once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904064172
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -11,8 +11,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import { NgModule } from "@angular/core";
-import { RouterModule, Routes } from "@angular/router";
+import {NgModule, Type} from "@angular/core";
+import {RouterModule, Routes} from "@angular/router";
Review Comment:
I think we really need to look into getting the linter to enforce the import style, it's very annoying having it be inconsistent (and I don't like my editors defaults either).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904016191
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -22,22 +24,37 @@ const routes: Routes = [
{
canLoad: [AuthenticatedGuard],
children: [{
- loadChildren: async (): Promise<object> => import("./core/core.module")
- .then(mod => mod.CoreModule),
+ loadChildren: async (): Promise<Type<unknown>> => {
+ const imp = import("./core/core.module");
+ return imp.then(mod => mod.CoreModule) as Promise<Type<typeof imp>>;
+ },
Review Comment:
I think the reason you made this a multi-line function block is because you needed to do the `as` statement to make the compiler happy. But you actually don't need to do that. Just change the return type from `object` to `Type<CoreModule>` (you'll need to import the `CoreModule` type). Same applies below to the `CustomModule` lazy load.
##########
experimental/traffic-portal/src/app/core/cache-groups/cache-group-table/cache-group-table.component.ts:
##########
@@ -132,7 +132,7 @@ export class CacheGroupTableComponent implements OnInit {
public fuzzySubject: BehaviorSubject<string>;
/** Form controller for the user search input. */
- public fuzzControl: FormControl = new FormControl("");
+ public fuzzControl: UntypedFormControl = new UntypedFormControl("");
Review Comment:
This is fine for now since this PR is big enough as-is, but we should make an issue to make these things typed. (Although honestly we're never using any actual FormControl features so most of these could probably use `ngModel` bindings instead).
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -11,8 +11,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import { NgModule } from "@angular/core";
-import { RouterModule, Routes } from "@angular/router";
+import {NgModule, Type} from "@angular/core";
+import {RouterModule, Routes} from "@angular/router";
Review Comment:
nit on line 14: `Type` is only used as a type, could be a `type` import
line 15: doesn't appear to have any content changes, could just be left alone
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.service.spec.ts:
##########
@@ -15,7 +15,7 @@ import { TestBed } from "@angular/core/testing";
import {TpHeaderComponent} from "src/app/shared/tp-header/tp-header.component";
-import { TpHeaderService } from "./tp-header.service";
+import {HeaderNavigation, TpHeaderService} from "./tp-header.service";
Review Comment:
nit: `HeaderNavigation` is only used as a type, could be a `type` import
although if you take my earlier advice and have the service own its default links this import will be unnecessary anyway.
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.html:
##########
@@ -21,7 +21,7 @@
<fa-icon [icon]="caretIcon" class="caret" [ngClass]="{'rotate': showMenu}"></fa-icon>
</button>
<mat-menu #menu="matMenu">
- <button mat-menu-item *ngFor="let c of columns; orderBy:'colDef.headerName'" (click)="toggleVisibility($event, c.getColId())">
+ <button mat-menu-item *ngFor="let c of columns" (click)="toggleVisibility($event, c.getColId())">
Review Comment:
I think the problem here is just that it's trying to order by `colDef.headerName` but the loop variable is `c`. Idk why that's compiling, frankly.
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.component.html:
##########
@@ -19,30 +19,36 @@ <h1>{{title ? title : 'Welcome to Traffic Portal!'}}</h1>
<div></div>
<nav id="expanded">
<ul>
- <li><a mat-button routerLink="/core/">Home</a></li>
- <li *ngIf="hasPermission('USER:READ')"><a mat-button routerLink="/core/users">Users</a></li>
- <li *ngIf="hasPermission('SERVER:READ')"><a mat-button routerLink="/core/servers">Servers</a></li>
+ <li *ngFor="let nav of horizNavs">
+ <a *ngIf="navShown(nav, 'anchor')" mat-button [routerLink]="nav.routerLink">{{nav.text}}</a>
+ <button *ngIf="navShown(nav, 'button')" mat-button (click)="nav?.click()">{{nav.text}}</button>
+ </li>
<li>
<button mat-icon-button [matMenuTriggerFor]="expandedMenu">
<mat-icon>manage_accounts</mat-icon>
</button>
<mat-menu #expandedMenu="matMenu">
- <a mat-menu-item routerLink="/core/me">Profile</a>
- <button mat-menu-item (click)="logout()">Logout</button>
<button mat-menu-item [matMenuTriggerFor]="themeMenu">Theme</button>
+ <div *ngFor="let nav of vertNavs">
+ <a *ngIf="navShown(nav, 'anchor')" mat-menu-item [routerLink]="nav.routerLink">{{nav.text}}</a>
+ <button *ngIf="navShown(nav, 'button')" mat-menu-item (click)="nav?.click()">{{nav.text}}</button>
Review Comment:
I'm pretty sure there's a way to do the typing so that button nav items aren't allowed to have a null/undefined `click` property. It seems really similar to what I did for the generic table's context menu items.
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.component.ts:
##########
@@ -37,17 +37,55 @@ export class TpHeaderComponent implements OnInit {
public hidden = false;
+ // Will try to display each of these navs on the header, space allowing.
+ public horizNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+ // Navs that are not directly displayed on the header.
+ public vertNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+
/**
* Angular lifecycle hook
*/
public ngOnInit(): void {
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core",
+ text: "Home",
+ type: "anchor",
+ }, "home");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/users",
+ text: "Users",
+ type: "anchor",
+ visible: () => this.hasPermission("USER:READ"),
+ }, "Users");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/servers",
+ text: "Servers",
+ type: "anchor",
+ visible: () => this.hasPermission("SERVER:READ"),
+ }, "Servers");
+ this.headerSvc.addVerticalNav({
+ routerLink: "/core/me",
+ text: "Profile",
+ type: "anchor"
+ }, "Profile");
+ this.headerSvc.addVerticalNav({
+ click: async () => this.logout(),
+ text: "Logout",
+ type: "anchor"
+ }, "Logout");
+
this.headerSvc.headerTitle.subscribe(title => {
this.title = title;
});
-
this.headerSvc.headerHidden.subscribe(hidden => {
this.hidden = hidden;
});
+ this.headerSvc.horizontalNavsUpdated.subscribe(navs => {
+ this.horizNavs = navs;
+ });
+ this.headerSvc.verticalNavsUpdated.subscribe(navs => {
+ this.vertNavs = navs;
+ });
Review Comment:
Instead of having the component attempt to re-insert these default links every time it renders - which will overwrite my overrides of default links - why not just have the service own them, which is a singleton by definition and won't overrwrite changes made through the service's API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 merged pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 merged PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r921472651
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.ts:
##########
@@ -327,19 +327,19 @@ export class GenericTableComponent<T> implements OnInit, OnDestroy {
}
try {
- const storedSort = localStorage.getItem(`${this.context}_table_sort`);
- if (storedSort) {
- this.gridAPI.setSortModel(JSON.parse(storedSort));
+ const filterState = localStorage.getItem(`${this.context}_table_filter`);
+ if (filterState) {
+ this.gridAPI.setFilterModel(JSON.parse(filterState));
Review Comment:
is Sort -> Filter a result of upgrading ag-grid? They renamed that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r921630187
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.ts:
##########
@@ -327,19 +327,19 @@ export class GenericTableComponent<T> implements OnInit, OnDestroy {
}
try {
- const storedSort = localStorage.getItem(`${this.context}_table_sort`);
- if (storedSort) {
- this.gridAPI.setSortModel(JSON.parse(storedSort));
+ const filterState = localStorage.getItem(`${this.context}_table_filter`);
+ if (filterState) {
+ this.gridAPI.setFilterModel(JSON.parse(filterState));
Review Comment:
Yes, sort is considered a part of column state ala `getColumnState`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r906322198
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -11,8 +11,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import { NgModule } from "@angular/core";
-import { RouterModule, Routes } from "@angular/router";
+import {NgModule, Type} from "@angular/core";
+import {RouterModule, Routes} from "@angular/router";
Review Comment:
I agree. I'm not sure eslint can do that or if we'd need prettier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904094710
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.html:
##########
@@ -21,7 +21,7 @@
<fa-icon [icon]="caretIcon" class="caret" [ngClass]="{'rotate': showMenu}"></fa-icon>
</button>
<mat-menu #menu="matMenu">
- <button mat-menu-item *ngFor="let c of columns; orderBy:'colDef.headerName'" (click)="toggleVisibility($event, c.getColId())">
+ <button mat-menu-item *ngFor="let c of columns" (click)="toggleVisibility($event, c.getColId())">
Review Comment:
Since it was invalid, the console was throwing warning (which is how I found it). Changing it to `c.headerName` makes it throw an error instead.
```
NG0303: Can't bind to 'ngForOrderBy' since it isn't a known property of 'button' (used in the 'GenericTableComponent' component template).
1. If 'button' is an Angular component and it has the 'ngForOrderBy' input, then verify that it is a part of an @NgModule where this component is declared.
2. To allow any property add 'NO_ERRORS_SCHEMA' to the '@NgModule.schemas' of this component.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904063343
##########
experimental/traffic-portal/src/app/core/cache-groups/cache-group-table/cache-group-table.component.ts:
##########
@@ -132,7 +132,7 @@ export class CacheGroupTableComponent implements OnInit {
public fuzzySubject: BehaviorSubject<string>;
/** Form controller for the user search input. */
- public fuzzControl: FormControl = new FormControl("");
+ public fuzzControl: UntypedFormControl = new UntypedFormControl("");
Review Comment:
I agree, these were all done by migrations so I left them be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6914: Add Ability to Extend TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r906313041
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.html:
##########
@@ -21,7 +21,7 @@
<fa-icon [icon]="caretIcon" class="caret" [ngClass]="{'rotate': showMenu}"></fa-icon>
</button>
<mat-menu #menu="matMenu">
- <button mat-menu-item *ngFor="let c of columns; orderBy:'colDef.headerName'" (click)="toggleVisibility($event, c.getColId())">
+ <button mat-menu-item *ngFor="let c of columns" (click)="toggleVisibility($event, c.getColId())">
Review Comment:
Yeah I'm seeing that too, looks like they dropped support for ordering in NgFor in favor of just having you pass the collection in the order you want it. I'm not sure why, since that's not trivial for asynchronous data sources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org