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 21:32:19 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6544: Layered profile `servers/` API {GET, POST, PUT & DELETE}
ocket8888 commented on a change in pull request #6544:
URL: https://github.com/apache/trafficcontrol/pull/6544#discussion_r827389333
##########
File path: blueprints/layered-profile-server.md
##########
@@ -93,7 +93,7 @@ With Layered Profiles, hundreds of profiles become a few dozen, each representin
{
"response": [{
"id": 5,
- "profiles": ["MID", "AMIGA_123", "CDN_FOO"]
+ "profileNames": ["MID", "AMIGA_123", "CDN_FOO"]
Review comment:
why'd we switch this? What's wrong with `profiles`?
##########
File path: lib/go-tc/servers.go
##########
@@ -24,6 +24,7 @@ import (
"encoding/json"
"errors"
"fmt"
+ "github.com/lib/pq"
Review comment:
third-party imports should be grouped last
##########
File path: lib/go-tc/servers.go
##########
@@ -718,6 +757,15 @@ type ServerNullableV2 struct {
IP6IsService *bool `json:"ip6IsService" db:"ip6_address_is_service"`
}
+type ServerNullableV40 struct {
Review comment:
Why'd you make a "Nullable" variant of the existing `ServerV40` type?
##########
File path: lib/go-tc/servers.go
##########
@@ -718,6 +757,15 @@ type ServerNullableV2 struct {
IP6IsService *bool `json:"ip6IsService" db:"ip6_address_is_service"`
}
+type ServerNullableV40 struct {
+ LegacyInterfaceDetails
Review comment:
APIv4 server structures should not use this legacy representation of network interface details
##########
File path: lib/go-tc/servers.go
##########
@@ -700,6 +701,44 @@ type CommonServerProperties struct {
XMPPPasswd *string `json:"xmppPasswd" db:"xmpp_passwd"`
}
+type CommonServerPropertiesV40 struct {
+ Cachegroup *string `json:"cachegroup" db:"cachegroup"`
+ CachegroupID *int `json:"cachegroupId" db:"cachegroup_id"`
+ CDNID *int `json:"cdnId" db:"cdn_id"`
+ CDNName *string `json:"cdnName" db:"cdn_name"`
+ DeliveryServices *map[string][]string `json:"deliveryServices,omitempty"`
+ DomainName *string `json:"domainName" db:"domain_name"`
+ FQDN *string `json:"fqdn,omitempty"`
+ FqdnTime time.Time `json:"-"`
+ GUID *string `json:"guid" db:"guid"`
+ HostName *string `json:"hostName" db:"host_name"`
+ HTTPSPort *int `json:"httpsPort" db:"https_port"`
+ ID *int `json:"id" db:"id"`
+ ILOIPAddress *string `json:"iloIpAddress" db:"ilo_ip_address"`
+ ILOIPGateway *string `json:"iloIpGateway" db:"ilo_ip_gateway"`
+ ILOIPNetmask *string `json:"iloIpNetmask" db:"ilo_ip_netmask"`
+ ILOPassword *string `json:"iloPassword" db:"ilo_password"`
+ ILOUsername *string `json:"iloUsername" db:"ilo_username"`
+ LastUpdated *TimeNoMod `json:"lastUpdated" db:"last_updated"`
+ MgmtIPAddress *string `json:"mgmtIpAddress" db:"mgmt_ip_address"`
+ MgmtIPGateway *string `json:"mgmtIpGateway" db:"mgmt_ip_gateway"`
+ MgmtIPNetmask *string `json:"mgmtIpNetmask" db:"mgmt_ip_netmask"`
+ OfflineReason *string `json:"offlineReason" db:"offline_reason"`
+ PhysLocation *string `json:"physLocation" db:"phys_location"`
+ PhysLocationID *int `json:"physLocationId" db:"phys_location_id"`
+ Profiles *pq.StringArray `json:"profileNames" db:"profile_names"`
Review comment:
It's normal for the Go structure name and JSON tags to match except for case differences. IMO, the JSON tag ought to change to be simply `profiles`, but if you feel strongly that `profileNames` is better then the Go struct property name should just change to `ProfileNames`.
##########
File path: traffic_ops/app/db/migrations/2022011314075900_add_server_profile.up.sql
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS public.server_profile (
+ server bigint NOT NULL,
+ profile_names text[] NOT NULL,
+ priority bigint[] NOT NULL,
+ CONSTRAINT pk_server_profile PRIMARY KEY(profile_names, server),
+ CONSTRAINT fk_server_id FOREIGN KEY (server) REFERENCES public.server(id) ON DELETE CASCADE ON UPDATE CASCADE
+);
+
+INSERT into public.server_profile(server, profile_names, priority)
+ SELECT s.id, ARRAY [p.name], ARRAY [0]
+ FROM public.server AS s
+ JOIN public.profile p ON p.id=s.profile;
Review comment:
missing newline at EOF
##########
File path: traffic_portal/app/src/common/modules/form/server/form.server.tpl.html
##########
@@ -112,11 +112,11 @@
<label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
<div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileId" ng-options="profile.id as profile.name for profile in profiles" required>
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileName" ng-options="profile.name as profile.name for profile in profiles" required>
Review comment:
we no longer need `as profile.name` here, since we're no longer valuing and labeling by different properties.
##########
File path: traffic_portal/app/src/common/modules/table/servers/TableServersController.js
##########
@@ -365,6 +365,9 @@ var TableServersController = function(tableName, servers, filter, $scope, $state
x.lastUpdated = x.lastUpdated ? new Date(x.lastUpdated.replace("+00", "Z")) : x.lastUpdated;
x.statusLastUpdated = x.statusLastUpdated ? new Date(x.statusLastUpdated): x.statusLastUpdated;
Object.assign(x, serverUtils.toLegacyIPInfo(x.interfaces));
+ if (x.profileNames != undefined) {
Review comment:
comparison should use `===`
##########
File path: traffic_portal/app/src/common/modules/form/server/form.server.tpl.html
##########
@@ -112,11 +112,11 @@
<label for="profile" ng-class="{'has-error': hasError(serverForm.profile)}">Profile *</label>
<div ng-class="{'has-error': hasError(serverForm.profile), 'has-feedback': hasError(serverForm.profile)}">
- <select id="profile" name="profile" class="form-control" ng-model="server.profileId" ng-options="profile.id as profile.name for profile in profiles" required>
+ <select id="profile" name="profile" class="form-control" ng-model="server.profileName" ng-options="profile.name as profile.name for profile in profiles" required>
<option hidden selected disabled value="">Select...</option>
</select>
<small class="input-error" ng-show="hasPropertyError(serverForm.profile, 'required')">Required</small>
- <small ng-show="server.profileId"><a ng-href="/#!/profiles/{{server.profileId}}" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
+ <small ng-show="server.profileNames.length>0"><a ng-href="getProfileID(server.profileNames[0])" target="_blank">View Details <i class="fa fs-xs fa-external-link"></i></a></small>
Review comment:
I don't believe that `getProfileID(server.profileNames[0])` will return a valid relative or absolute URL.
##########
File path: traffic_portal/app/src/common/modules/form/server/edit/FormEditServerController.js
##########
@@ -45,6 +45,7 @@ var FormEditServerController = function(server, $anchorScroll, $scope, $controll
};
$scope.save = function(server) {
+ console.log(server.profileNames = [server.profileName]);
Review comment:
This assignment has no return value, so this will only and always ever print `undefined` to the console. It shouldn't be logged.
##########
File path: lib/go-tc/servers.go
##########
@@ -1005,6 +1005,47 @@ func (s ServerNullableV2) UpgradeToV40(cspV40 CommonServerPropertiesV40) (Server
return upgraded, nil
}
+// UpdateCommonServerPropertiesV40 updates CommonServerProperties of V2 and V3 to CommonServerPropertiesV40
+func UpdateCommonServerPropertiesV40(profileName pq.StringArray, properties CommonServerProperties) CommonServerPropertiesV40 {
Review comment:
Does this need to take in a Profile Name as an argument? `CommonServerProperties` contains the `ProfileName` field.
##########
File path: traffic_portal/app/src/common/modules/form/server/FormServerController.js
##########
@@ -60,6 +60,14 @@ var FormServerController = function(server, $scope, $location, $state, $uibModal
});
};
+ $scope.getProfileID = function(profileName) {
+ for (let profile of $scope.profiles) {
Review comment:
nit: `profile` is never reassigned - could be `const` instead.
##########
File path: traffic_portal/app/src/common/modules/form/server/FormServerController.js
##########
@@ -60,6 +60,14 @@ var FormServerController = function(server, $scope, $location, $state, $uibModal
});
};
+ $scope.getProfileID = function(profileName) {
+ for (let profile of $scope.profiles) {
+ if (profile.name == profileName) {
Review comment:
comparison should use `===`
##########
File path: lib/go-tc/servers.go
##########
@@ -718,6 +757,15 @@ type ServerNullableV2 struct {
IP6IsService *bool `json:"ip6IsService" db:"ip6_address_is_service"`
}
+type ServerNullableV40 struct {
+ LegacyInterfaceDetails
+ CommonServerPropertiesV40
+ RouterHostName *string `json:"routerHostName" db:"router_host_name"`
+ RouterPortName *string `json:"routerPortName" db:"router_port_name"`
+ IPIsService *bool `json:"ipIsService" db:"ip_address_is_service"`
+ IP6IsService *bool `json:"ip6IsService" db:"ip6_address_is_service"`
Review comment:
This is reverting changes from nearly two years ago in #4612 - I don't think we should do that?
##########
File path: lib/go-tc/servers.go
##########
@@ -718,6 +757,15 @@ type ServerNullableV2 struct {
IP6IsService *bool `json:"ip6IsService" db:"ip6_address_is_service"`
}
+type ServerNullableV40 struct {
+ LegacyInterfaceDetails
+ CommonServerPropertiesV40
+ RouterHostName *string `json:"routerHostName" db:"router_host_name"`
+ RouterPortName *string `json:"routerPortName" db:"router_port_name"`
Review comment:
This is reverting a change from more than a year ago in #5450 - I don't think we should do that.
##########
File path: traffic_ops/app/db/migrations/2022011314075900_add_server_profile.up.sql
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.
+ */
+
+CREATE TABLE IF NOT EXISTS public.server_profile (
+ server bigint NOT NULL,
+ profile_names text[] NOT NULL,
+ priority bigint[] NOT NULL,
+ CONSTRAINT pk_server_profile PRIMARY KEY(profile_names, server),
+ CONSTRAINT fk_server_id FOREIGN KEY (server) REFERENCES public.server(id) ON DELETE CASCADE ON UPDATE CASCADE
+);
Review comment:
Instead of
```
Table "public.server_profile"
Column | Type | Collation | Nullable | Default
---------------+----------+-----------+----------+---------
server | bigint | | not null |
profile_names | text[] | | not null |
priority | bigint[] | | not null |
Indexes:
"pk_server_profile" PRIMARY KEY, btree (profile_names, server)
Foreign-key constraints:
"fk_server_id" FOREIGN KEY (server) REFERENCES server(id) ON UPDATE CASCADE ON DELETE CASCADE
```
Why not just
```
Table "public.server_profile"
Column | Type | Collation | Nullable | Default
----------+--------+-----------+----------+---------
server | bigint | | not null |
profile | text | | not null |
priority | bigint | | not null |
Indexes:
"pk_server_profile" PRIMARY KEY, btree (profile, server)
"server_profile_unique_priority" UNIQUE CONSTRAINT, btree (server, priority)
Foreign-key constraints:
"fk_server_profile_profile" FOREIGN KEY (profile) REFERENCES profile(name) ON UPDATE CASCADE ON DELETE RESTRICT
"fk_server_profile_server" FOREIGN KEY (server) REFERENCES server(id) ON UPDATE CASCADE ON DELETE CASCADE
```
It's easier to work with, gives a slightly better guarantee of well-formed priorities (doesn't allow duplicates for the same server), and guarantees binding of Profile Names to actual Profiles - that is, under your current implementation, servers can have entries in their `profileNames` that aren't the names of any existing Profiles. This will happen if, for instance, the Name of a Profile is altered once it's been assigned to one or more Servers.
--
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