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/03/15 19:15:42 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6639: Added Traffic Portal page for CDNi config requests
ocket8888 commented on a change in pull request #6639:
URL: https://github.com/apache/trafficcontrol/pull/6639#discussion_r827273228
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -789,3 +833,12 @@ type CapacityLimit struct {
LimitValue int64 `json:"limit-value"`
Footprints []Footprint `json:"footprints"`
}
+
+type ConfigurationUpdateRequest struct {
+ Id int `json:"id"`
Review comment:
nit: `Id` should be `ID`.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -789,3 +833,12 @@ type CapacityLimit struct {
LimitValue int64 `json:"limit-value"`
Footprints []Footprint `json:"footprints"`
}
+
+type ConfigurationUpdateRequest struct {
Review comment:
GoDoc?
##########
File path: traffic_portal/app/src/app.js
##########
@@ -212,6 +212,9 @@ var trafficPortal = angular.module('trafficPortal', [
require('./modules/private/tenants/users').name,
require('./modules/private/certExpirations').name,
require('./modules/private/certExpirations/list').name,
+ require('./modules/private/cdniConfigRequests').name,
+ require('./modules/private/cdniConfigRequests/list').name,
+ require('./modules/private/cdniConfigRequests/view').name,
Review comment:
The rest of this file (unfortunately) uses spaces instead of indentation. It should be consistent - but if you want to convert the whole file to _real_ indentation instead, I'm 100% behind that.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -789,3 +833,12 @@ type CapacityLimit struct {
LimitValue int64 `json:"limit-value"`
Footprints []Footprint `json:"footprints"`
}
+
+type ConfigurationUpdateRequest struct {
+ Id int `json:"id"`
+ UCDN string `json:"ucdn"`
+ Data json.RawMessage `json:"data"`
+ Host string `json:"host"`
+ RequestType string `json:"request_type"`
+ AsyncStatusId int `json:"async_status_id"`
Review comment:
nit: `AsyncStatusId` should be `AsyncStatusID`
##########
File path: traffic_portal/test/integration/PageObjects/SideNavigationPage.po.ts
##########
@@ -143,6 +144,10 @@ export class SideNavigationPage extends BasePage{
await browser.actions().mouseMove(this.lnkStatuses).perform();
await browser.actions().click(this.lnkStatuses).perform();
}
+ /** NavigateToCdniRequestsPage verifies that the link to the CDNi Requests page is clickable. */
+ public async NavigateToCdniRequestsPage(): Promise<void>{
+ return this.lnkCdniConfigRequests.click();
+ }
Review comment:
This method is never used - were you going to write tests for this?
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/FormCdniRequestController.js
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+var FormCdniRequestController = function($scope, $stateParams, $uibModal, cdniService, cdniRequest, currentConfig, locationUtils, messageModel) {
+ $scope.reqId = $stateParams.reqId;
+ $scope.cdniRequest = cdniRequest;
+ $scope.cdniRequest.data = JSON.stringify($scope.cdniRequest.data, null, 5);
+ $scope.currentConfig = JSON.stringify(currentConfig, null, 5);
+
+ $scope.navigateToPath = locationUtils.navigateToPath;
+
+ $scope.respondToRequest = function(approve) {
+ let titleStart = approve ? 'Approve' : 'Deny';
+ var params = {
Review comment:
nit: `params` is never reassigned. Could use `const` instead.
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/form.cdniConfigRequests.tpl.html
##########
@@ -0,0 +1,53 @@
+<!--
+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.
+-->
+
+<div class="x_panel">
+ <div class="x_title">
+ <ol class="breadcrumb pull-left">
+ <li><a ng-click="navigateToPath('/cdni-config-requests')">CDNi Requests</a></li>
Review comment:
anchor elements should use hyperrefs, not `'click'` event listeners.
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/FormCdniRequestController.js
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+var FormCdniRequestController = function($scope, $stateParams, $uibModal, cdniService, cdniRequest, currentConfig, locationUtils, messageModel) {
+ $scope.reqId = $stateParams.reqId;
+ $scope.cdniRequest = cdniRequest;
+ $scope.cdniRequest.data = JSON.stringify($scope.cdniRequest.data, null, 5);
+ $scope.currentConfig = JSON.stringify(currentConfig, null, 5);
+
+ $scope.navigateToPath = locationUtils.navigateToPath;
+
+ $scope.respondToRequest = function(approve) {
+ let titleStart = approve ? 'Approve' : 'Deny';
+ var params = {
+ title: titleStart + ' CDNi Update Request: ' + cdniRequest.id
+ };
+ var modalInstance = $uibModal.open({
Review comment:
nit: `modalInstance` is never reassigned. Could be `const` instead.
##########
File path: docs/source/api/v4/oc_fci_configuration_requests.rst
##########
@@ -0,0 +1,96 @@
+..
+..
+.. Licensed 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.
+..
+
+.. _to-api-oc-fci-configuration_requests:
+
+*********************************
+``OC/FCI/configuration/requests``
+*********************************
+
+``GET``
+=======
+Returns the requested updates for :abbr:`CDNi (Content Delivery Network Interconnect)` configurations. An optional ``id`` parameter will return only information for a specific request.
+
+:Auth. Required: Yes
+:Roles Required: "admin"
+:Permissions Required: CDNI-CAPACITY:ADMIN
+:Response Type: Array
+
+Request Structure
+-----------------
+.. table:: Request Query Parameters
+
+ +-----------+----------+---------------------------------------------------------------------------------------------------------------+
+ | Name | Required | Description |
+ +===========+==========+===============================================================================================================+
+ | id | no | Return only the configuration requests identified by this integral, unique identifier |
+ +-----------+----------+---------------------------------------------------------------------------------------------------------------+
+
+Response Structure
+------------------
+:id: An integral, unique identifier for the requested configuration updates.
+:ucdn: The name of the :abbr:`uCDN (Upstream Content Delivery Network)` to which the requested changes apply.
+:data: An array of generic :abbr:`FCI (Footprint and Capabilities Advertisement Interface)` base objects.
+:host: The domain to which the requested changes apply.
+:request_type: A string of the type of configuration update request.
+:async_status_id: An integral, unique identifier for the associated asynchronous status.
+:capability-type: A string of the type of base object.
+:capability-value: An array of the value for the base object.
Review comment:
does the spec really require mixing kebab-case and snake_case like this?
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/form.cdniConfigRequests.tpl.html
##########
@@ -0,0 +1,53 @@
+<!--
+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.
+-->
+
+<div class="x_panel">
+ <div class="x_title">
+ <ol class="breadcrumb pull-left">
+ <li><a ng-click="navigateToPath('/cdni-config-requests')">CDNi Requests</a></li>
+ <li class="active">{{reqId}}</li>
+ </ol>
+ <div class="clearfix"></div>
+ </div>
+ <div class="x_content">
+ <br>
+ <form name="cdniRequestForm" class="form-horizontal form-label-left" novalidate>
+ <div class="form-group">
+ <label for="newData" class="control-label col-md-2 col-sm-2 col-xs-12">Updated Data</label>
+ <div class="col-md-10 col-sm-10 col-xs-12" style="padding-top: 10px">
+ <div class="col-md-10 col-sm-10 col-xs-12">
+ <textarea id="newData" name="newData" type="text" class="form-control" ng-model="cdniRequest.data" rows="25" readonly></textarea>
Review comment:
I don't think this is meant to be `readonly`
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/FormCdniRequestController.js
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+var FormCdniRequestController = function($scope, $stateParams, $uibModal, cdniService, cdniRequest, currentConfig, locationUtils, messageModel) {
+ $scope.reqId = $stateParams.reqId;
+ $scope.cdniRequest = cdniRequest;
+ $scope.cdniRequest.data = JSON.stringify($scope.cdniRequest.data, null, 5);
+ $scope.currentConfig = JSON.stringify(currentConfig, null, 5);
+
+ $scope.navigateToPath = locationUtils.navigateToPath;
+
+ $scope.respondToRequest = function(approve) {
+ let titleStart = approve ? 'Approve' : 'Deny';
+ var params = {
+ title: titleStart + ' CDNi Update Request: ' + cdniRequest.id
+ };
+ var modalInstance = $uibModal.open({
+ templateUrl: 'common/modules/dialog/confirm/dialog.confirm.tpl.html',
+ controller: 'DialogConfirmController',
+ size: 'md',
+ resolve: {
+ params: function () {
+ return params;
+ }
+ }
Review comment:
nit: resolution of identity function is equivalent to identity itself e.g.
```javascript
const modalInstance = $uibModal.open({
templateUrl: "common/modules/dialog/confirm/dialog.confirmtpl.html",
controller: "DialogConfirmController",
size: "md",
resolve: {params}
});
```
(that example also uses ES6 object literal property value shorthand - `{params: params}` is equivalent to `{params}`)
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/FormCdniRequestController.js
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+var FormCdniRequestController = function($scope, $stateParams, $uibModal, cdniService, cdniRequest, currentConfig, locationUtils, messageModel) {
+ $scope.reqId = $stateParams.reqId;
+ $scope.cdniRequest = cdniRequest;
+ $scope.cdniRequest.data = JSON.stringify($scope.cdniRequest.data, null, 5);
+ $scope.currentConfig = JSON.stringify(currentConfig, null, 5);
+
+ $scope.navigateToPath = locationUtils.navigateToPath;
+
+ $scope.respondToRequest = function(approve) {
+ let titleStart = approve ? 'Approve' : 'Deny';
Review comment:
nit: `titleStart` is never reassigned. Could use `const` instead.
##########
File path: traffic_ops/traffic_ops_golang/cdni/shared.go
##########
@@ -272,6 +274,48 @@ func PutConfiguration(w http.ResponseWriter, r *http.Request) {
api.WriteAlerts(w, r, http.StatusAccepted, alerts)
}
+func GetRequests(w http.ResponseWriter, r *http.Request) {
Review comment:
GoDoc?
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -136,6 +136,7 @@ func Routes(d ServerData) ([]Route, http.Handler, error) {
{Version: api.Version{Major: 4, Minor: 0}, Method: http.MethodPut, Path: `OC/CI/configuration/?$`, Handler: cdni.PutConfiguration, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"CDNI-CAPACITY:UPDATE"}, Authenticated: Authenticated, Middlewares: nil, ID: 541357729078},
{Version: api.Version{Major: 4, Minor: 0}, Method: http.MethodPut, Path: `OC/CI/configuration/{host}?$`, Handler: cdni.PutHostConfiguration, RequiredPrivLevel: auth.PrivLevelReadOnly, RequiredPermissions: []string{"CDNI-CAPACITY:UPDATE"}, Authenticated: Authenticated, Middlewares: nil, ID: 541357729079},
{Version: api.Version{Major: 4, Minor: 0}, Method: http.MethodPut, Path: `OC/CI/configuration/request/{id}/{approved}?$`, Handler: cdni.PutConfigurationResponse, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"CDNI-CAPACITY:ADMIN"}, Authenticated: Authenticated, Middlewares: nil, ID: 541357729080},
+ {Version: api.Version{Major: 4, Minor: 0}, Method: http.MethodGet, Path: `OC/CI/configuration/requests?$`, Handler: cdni.GetRequests, RequiredPrivLevel: auth.PrivLevelAdmin, RequiredPermissions: []string{"CDNI-CAPACITY:ADMIN"}, Authenticated: Authenticated, Middlewares: nil, ID: 541357729081},
Review comment:
the `s` at the end of `OC/CI/configuration/requests` should not be optional. Matter of fact, idk what the `?` does to the two patterns directly above this line.
##########
File path: traffic_portal/app/src/common/modules/form/cdniConfigRequests/FormCdniRequestController.js
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+
+var FormCdniRequestController = function($scope, $stateParams, $uibModal, cdniService, cdniRequest, currentConfig, locationUtils, messageModel) {
+ $scope.reqId = $stateParams.reqId;
+ $scope.cdniRequest = cdniRequest;
+ $scope.cdniRequest.data = JSON.stringify($scope.cdniRequest.data, null, 5);
+ $scope.currentConfig = JSON.stringify(currentConfig, null, 5);
+
+ $scope.navigateToPath = locationUtils.navigateToPath;
+
+ $scope.respondToRequest = function(approve) {
+ let titleStart = approve ? 'Approve' : 'Deny';
+ var params = {
+ title: titleStart + ' CDNi Update Request: ' + cdniRequest.id
Review comment:
nit: good use-case for template string like
```javascript
const params = {
title: `${titleStart} CDNi Update Request: ${cdniRequest.id}`
};
```
##########
File path: traffic_portal/app/src/common/modules/navigation/navigation.tpl.html
##########
@@ -50,6 +50,7 @@
<li class="side-menu-category-item" ng-if="hasCapability('params-read')" ng-class="{'current-page': isState('trafficPortal.private.parameters')}"><a href="/#!/parameters">Parameters</a></li>
<li class="side-menu-category-item" ng-if="hasCapability('types-read')" ng-class="{'current-page': isState('trafficPortal.private.types')}"><a href="/#!/types">Types</a></li>
<li class="side-menu-category-item" ng-if="hasCapability('statuses-read')" ng-class="{'current-page': isState('trafficPortal.private.statuses')}"><a href="/#!/statuses">Statuses</a></li>
+ <li class="side-menu-category-item" ng-if="hasCapability('cdni-capacity-admin')" ng-class="{'current-page': isState('trafficPortal.private.cdniConfigRequests')}"><a href="/#!/cdni-config-requests">CDNi Requests</a></li>
Review comment:
This restriction that the user have the "capability" `cdni-capacity-admin` is not the same as requiring that they have the `CDNI-CAPACITY:ADMIN` Permission. I'd recommend in TPv1 you don't check "capabilities"/Permissions at all.
##########
File path: docs/source/api/v4/oc_fci_configuration_requests.rst
##########
@@ -0,0 +1,96 @@
+..
+..
+.. Licensed 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.
+..
+
+.. _to-api-oc-fci-configuration_requests:
+
+*********************************
+``OC/FCI/configuration/requests``
+*********************************
+
+``GET``
+=======
+Returns the requested updates for :abbr:`CDNi (Content Delivery Network Interconnect)` configurations. An optional ``id`` parameter will return only information for a specific request.
+
+:Auth. Required: Yes
+:Roles Required: "admin"
+:Permissions Required: CDNI-CAPACITY:ADMIN
Review comment:
Common practice is to use <code><var>THING</var>:READ</code> as the required permission for read-only operations on <var>THING</var>. That seems appropriate here, since this isn't doing any administration.
##########
File path: traffic_portal/app/src/common/modules/table/cdniConfigRequests/TableCdniController.js
##########
@@ -0,0 +1,68 @@
+/*
+ * 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.
+ */
+
+var TableCdniController = function(tableName, cdniRequests, $scope, locationUtils) {
+
+ $scope.cdniRequests = cdniRequests.map(
+ function(x) {
+ // need to convert this to a date object for ag-grid filter to work properly
+ x.data = JSON.stringify(x.data);
+ return x;
+ });
+
+ /** The columns of the ag-grid table */
+ $scope.columns = [
+ {
+ headerName: "Upstream CDN",
+ field: "ucdn",
+ hide: false
+ },
+ {
+ headerName: "Host",
+ field: "host",
+ hide: false
+ },
+ {
+ headerName: "Request Type",
+ field: "request_type",
+ hide: false
+ },
+ {
+ headerName: "New Data",
Review comment:
Isn't this the "current configuration"?
--
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