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 2020/07/08 03:03:54 UTC

[GitHub] [trafficcontrol] rob05c opened a new pull request #4861: Add ORT Inferring Location Parameters

rob05c opened a new pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861


   Adds ORT location Parameter inference. If location Parameters do not exist
   the files are created and added anyway with the directory determined
   from the local ATS install, for:
   - cache.config
   - hosting.config
   - ip_allow.config
   - parent.config
   - plugin.config
   - records.config
   - remap.config
   - storage.config
   - volume.config
   - Delivery Services with
     - Edge Header Rewrite
     - Mid Header Rewrite,
     - Cache URL
     - URL Sig
     - URI Signing
   
   Note this is not an exhaustive of required files, and many files
   must be dynamic and cannot be inferred.
   
   But this does remove the unnecessary manual configuration for this list.
   More files may be added in the future.
   
   - [x] This PR is not related to any other Issue
   
   I've manually tested in a production-like environment, verified ORT places server-wide configs like remap.config, as well as DS-specific configs like hdr_rw_dsname.config, without `location` Parameters.
   
   Includes tests.
   No docs, location Parameters aren't documented (maybe they should be, but that's outside the scope of this PR).
   Includes changelog.
   
   ## Which Traffic Control components are affected by this PR?
   - Traffic Ops ORT
   
   ## What is the best way to verify this PR?
   Run unit tests.
   Run ORT against a Traffic Ops as a Server with no location Parameter on its Profile for remap.config, hdr_rw_dsname.config, and others in the above list, verify configs are still placed properly.
   
   ## If this is a bug fix, what versions of Traffic Control are affected?
   Not a bug fix.
   
   ## The following criteria are ALL met by this PR
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   ## Additional Information
   
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-659494608


   retest this please


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-659695086


   retest this please


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655592369


   Also, once this is merged, we should make a new Feature/TechDebt Issue to change Traffic Ops Delivery Service creation and modification to not create or update `location` Parameters.
   
   I thought about doing that here, but it seems safer and smaller to do them separately. And it doesn't hurt anything to keep creating them.
   
   Should be straightforward to remove. Code is here: https://github.com/apache/trafficcontrol/blob/83d3012eec70646c958225025a364650b9aea99c/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go#L1476


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655662775


   ATS Install. It tries `rpm` and falls back to `find`: https://github.com/apache/trafficcontrol/blob/4aa550d319c0269f8e9fbd8fabc0aa757da1a5f9/traffic_ops_ort/traffic_ops_ort.pl#L275
   
   That's important for users who may have ATS installed somewhere other than `/opt/trafficserver`. Which is perfectly reasonable: The Linux Standard Base makes it clear `/opt` is for people who don't play well with others, standard LSB directories (`/usr/sbin`, `/etc`, `/var/log`, etc are preferable.
   
   I believe that's where ATS makefiles install by default, as well.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r456032530



##########
File path: traffic_ops_ort/traffic_ops_ort.pl
##########
@@ -1583,7 +1605,7 @@ sub get_cfg_file_list {
 		$atstccfg_reval_arg = '--revalidate-only';
 	}
 
-	my $result = `$atstccfg_cmd $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
+	my $result = `$atstccfg_cmd --dir='$ats_config_dir' $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;

Review comment:
       Oh, okay, I must've been looking at a subset of changes or something.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-660146344


   Not anywhere. There are still both required and custom ("take-n-bake") locations that are necessary.
   
   But any in that list above, yes. I'll make that case, as well as one for the CiaB ort.py inferring /etc/trafficserver


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-660212529


   Issues created.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655669673


   Should also be easy to add other package manager like `apt` in the future


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r455997552



##########
File path: traffic_ops_ort/traffic_ops_ort.pl
##########
@@ -1583,7 +1605,7 @@ sub get_cfg_file_list {
 		$atstccfg_reval_arg = '--revalidate-only';
 	}
 
-	my $result = `$atstccfg_cmd $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
+	my $result = `$atstccfg_cmd --dir='$ats_config_dir' $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;

Review comment:
       Yeah, config.go line 103




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r456000819



##########
File path: lib/go-atscfg/meta.go
##########
@@ -40,6 +41,23 @@ type ConfigProfileParams struct {
 // TODO change the config system to not use old API paths, and remove this.
 const APIVersion = "2.0"
 
+// RequiredFiles is a constant (because Go doesn't allow const slices).

Review comment:
       Fixed




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c removed a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c removed a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-659493926


   Retest this please.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-659694691


   +1
   
   I'd vote we remove the CiaB location params, as well as change TO to not create DS location params, in subsequent PRs, just to keep the PRs as small as possible


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-659493926


   Retest this please.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655662775


   ATS Install. It tries `rpm` and falls back to `find`: https://github.com/apache/trafficcontrol/blob/4aa550d319c0269f8e9fbd8fabc0aa757da1a5f9/traffic_ops_ort/traffic_ops_ort.pl#L275
   
   That's important for users who may have ATS installed somewhere other than `/opt/trafficserver`. Which is perfectly reasonable: The LSB makes it clear `/opt` is for people who don't play well with others, standard LSB directories (`/usr/sbin`, `/etc`, `/var/log`, etc.
   
   I believe that's where ATS makefiles install by default, as well.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r455999957



##########
File path: lib/go-atscfg/meta.go
##########
@@ -63,6 +87,146 @@ func MetaObjToMetaConfig(atsData tc.ATSConfigMetaData) string {
 	return string(bts)
 }
 
+// AddMetaObjConfigDir takes the Meta Object generated from TO data, and the ATS config directory
+// and prepends the config directory to all relative paths,
+// and creates MetaObj entries for all required config files which have no location parameter.
+// If configDir is empty and any location Parameters have relative paths, returns an error.
+func AddMetaObjConfigDir(
+	metaObj tc.ATSConfigMetaData,
+	configDir string,
+	serverHostName tc.CacheName,
+	server *ServerInfo,
+	tmURL string, // global tm.url Parameter
+	tmReverseProxyURL string, // global tm.rev_proxy.url Parameter
+	locationParams map[string]ConfigProfileParams, // map[configFile]params; 'location' and 'URL' Parameters on serverHostName's Profile
+	uriSignedDSes []tc.DeliveryServiceName,
+	scopeParams map[string]string, // map[configFileName]scopeParam
+	dses map[tc.DeliveryServiceName]tc.DeliveryServiceNullable,
+) (tc.ATSConfigMetaData, error) {
+
+	// Note there may be multiple files with the same name in different directories.
+	configFilesM := map[string][]tc.ATSConfigMetaDataConfigFile{} // map[fileShortName]tc.ATSConfigMetaDataConfigFile
+	for _, fi := range metaObj.ConfigFiles {
+		configFilesM[fi.FileNameOnDisk] = append(configFilesM[fi.FileNameOnDisk], fi)
+	}
+
+	// add all strictly required files, all of which should be in the base config directory.
+	// If they don't exist, create them.
+	// If they exist with a relative path, prepend configDir.
+	// If any exist with a relative path, or don't exist, and configDir is empty, return an error.
+	for _, fileName := range requiredFiles() {
+		if _, ok := configFilesM[fileName]; ok {
+			continue
+		}
+		if configDir == "" {
+			return metaObj, errors.New("required file '" + fileName + "' has no location Parameter, and ATS config directory not found.")
+		}
+		configFilesM[fileName] = []tc.ATSConfigMetaDataConfigFile{{
+			FileNameOnDisk: fileName,
+			Location:       configDir,
+			Scope:          string(getServerScope(fileName, server.Type, nil)),
+		}}
+	}
+
+	for fileName, fis := range configFilesM {
+		newFis := []tc.ATSConfigMetaDataConfigFile{}

Review comment:
       Performance doesn't matter here, and I prefer not to use `make` unless necessary to have a unified variable syntax for readability. But you're right, it is faster. I can change it if you feel strongly about it.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r455986929



##########
File path: lib/go-atscfg/meta.go
##########
@@ -63,6 +87,146 @@ func MetaObjToMetaConfig(atsData tc.ATSConfigMetaData) string {
 	return string(bts)
 }
 
+// AddMetaObjConfigDir takes the Meta Object generated from TO data, and the ATS config directory
+// and prepends the config directory to all relative paths,
+// and creates MetaObj entries for all required config files which have no location parameter.
+// If configDir is empty and any location Parameters have relative paths, returns an error.
+func AddMetaObjConfigDir(
+	metaObj tc.ATSConfigMetaData,
+	configDir string,
+	serverHostName tc.CacheName,
+	server *ServerInfo,
+	tmURL string, // global tm.url Parameter
+	tmReverseProxyURL string, // global tm.rev_proxy.url Parameter
+	locationParams map[string]ConfigProfileParams, // map[configFile]params; 'location' and 'URL' Parameters on serverHostName's Profile
+	uriSignedDSes []tc.DeliveryServiceName,
+	scopeParams map[string]string, // map[configFileName]scopeParam
+	dses map[tc.DeliveryServiceName]tc.DeliveryServiceNullable,
+) (tc.ATSConfigMetaData, error) {
+
+	// Note there may be multiple files with the same name in different directories.
+	configFilesM := map[string][]tc.ATSConfigMetaDataConfigFile{} // map[fileShortName]tc.ATSConfigMetaDataConfigFile
+	for _, fi := range metaObj.ConfigFiles {
+		configFilesM[fi.FileNameOnDisk] = append(configFilesM[fi.FileNameOnDisk], fi)
+	}
+
+	// add all strictly required files, all of which should be in the base config directory.
+	// If they don't exist, create them.
+	// If they exist with a relative path, prepend configDir.
+	// If any exist with a relative path, or don't exist, and configDir is empty, return an error.
+	for _, fileName := range requiredFiles() {
+		if _, ok := configFilesM[fileName]; ok {
+			continue
+		}
+		if configDir == "" {
+			return metaObj, errors.New("required file '" + fileName + "' has no location Parameter, and ATS config directory not found.")
+		}
+		configFilesM[fileName] = []tc.ATSConfigMetaDataConfigFile{{
+			FileNameOnDisk: fileName,
+			Location:       configDir,
+			Scope:          string(getServerScope(fileName, server.Type, nil)),
+		}}
+	}
+
+	for fileName, fis := range configFilesM {
+		newFis := []tc.ATSConfigMetaDataConfigFile{}

Review comment:
       Since you know ahead of time what the size of this needs to be, you can allocate: `newFis := make([]tc.ATSConfigMetaDataConfigFile, 0, len(fis))` to avoid possible multiple reallocations.

##########
File path: lib/go-atscfg/meta.go
##########
@@ -40,6 +41,23 @@ type ConfigProfileParams struct {
 // TODO change the config system to not use old API paths, and remove this.
 const APIVersion = "2.0"
 
+// RequiredFiles is a constant (because Go doesn't allow const slices).

Review comment:
       nit, but it's `requiredFiles`, not `RequiredFiles`.

##########
File path: traffic_ops_ort/traffic_ops_ort.pl
##########
@@ -1583,7 +1605,7 @@ sub get_cfg_file_list {
 		$atstccfg_reval_arg = '--revalidate-only';
 	}
 
-	my $result = `$atstccfg_cmd $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;
+	my $result = `$atstccfg_cmd --dir='$ats_config_dir' $atstccfg_timeout_arg $atstccfg_arg_disable_proxy --traffic-ops-user='$TO_USER' --traffic-ops-password='$TO_PASS' --traffic-ops-url='$TO_URL' --cache-host-name='$host_name' $atstccfg_reval_arg --log-location-error=stderr --log-location-warning=stderr --log-location-info=null 2>$atstccfg_log_path`;

Review comment:
       Am I missing something? It doesn't look like you added this option-argument to `traffic_ops_ort/atstccfg/config/config.go`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] mitchell852 commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-660132097


   @rob05c - can you create a github issue regarding the removal from TO of the creation of location params? I'm guessing that anywhere in the code that creates a location param can be removed, right?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655633894


   > _"If location Parameters do not exist the files are created and added anyway with the directory determined from the local ATS install"_
   
   Does it actually infer based on the local ATS install, or is it just hard-coded to use `/opt/trafficserver/` as the root of the ATS install? If it does infer, how?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#discussion_r456030974



##########
File path: lib/go-atscfg/meta.go
##########
@@ -63,6 +87,146 @@ func MetaObjToMetaConfig(atsData tc.ATSConfigMetaData) string {
 	return string(bts)
 }
 
+// AddMetaObjConfigDir takes the Meta Object generated from TO data, and the ATS config directory
+// and prepends the config directory to all relative paths,
+// and creates MetaObj entries for all required config files which have no location parameter.
+// If configDir is empty and any location Parameters have relative paths, returns an error.
+func AddMetaObjConfigDir(
+	metaObj tc.ATSConfigMetaData,
+	configDir string,
+	serverHostName tc.CacheName,
+	server *ServerInfo,
+	tmURL string, // global tm.url Parameter
+	tmReverseProxyURL string, // global tm.rev_proxy.url Parameter
+	locationParams map[string]ConfigProfileParams, // map[configFile]params; 'location' and 'URL' Parameters on serverHostName's Profile
+	uriSignedDSes []tc.DeliveryServiceName,
+	scopeParams map[string]string, // map[configFileName]scopeParam
+	dses map[tc.DeliveryServiceName]tc.DeliveryServiceNullable,
+) (tc.ATSConfigMetaData, error) {
+
+	// Note there may be multiple files with the same name in different directories.
+	configFilesM := map[string][]tc.ATSConfigMetaDataConfigFile{} // map[fileShortName]tc.ATSConfigMetaDataConfigFile
+	for _, fi := range metaObj.ConfigFiles {
+		configFilesM[fi.FileNameOnDisk] = append(configFilesM[fi.FileNameOnDisk], fi)
+	}
+
+	// add all strictly required files, all of which should be in the base config directory.
+	// If they don't exist, create them.
+	// If they exist with a relative path, prepend configDir.
+	// If any exist with a relative path, or don't exist, and configDir is empty, return an error.
+	for _, fileName := range requiredFiles() {
+		if _, ok := configFilesM[fileName]; ok {
+			continue
+		}
+		if configDir == "" {
+			return metaObj, errors.New("required file '" + fileName + "' has no location Parameter, and ATS config directory not found.")
+		}
+		configFilesM[fileName] = []tc.ATSConfigMetaDataConfigFile{{
+			FileNameOnDisk: fileName,
+			Location:       configDir,
+			Scope:          string(getServerScope(fileName, server.Type, nil)),
+		}}
+	}
+
+	for fileName, fis := range configFilesM {
+		newFis := []tc.ATSConfigMetaDataConfigFile{}

Review comment:
       I guess it doesn't really matter.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655592369


   Also, once this is merged, we should make a new Feature Issue to change Traffic Ops Delivery Service creation and modification to not create or update `location` Parameters.
   
   I thought about doing that here, but it seems safer and smaller to do them separately. And it doesn't hurt anything to keep creating them.
   
   Should be straightforward to remove. Code is here: https://github.com/apache/trafficcontrol/blob/83d3012eec70646c958225025a364650b9aea99c/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go#L1476


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655669673


   Should also be easy to add other package manager like `apt` in the future.
   And the `find` fallback hopefully works on any POSIX-like OS (though I haven't tested).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655662775


   ATS Install. It tries `rpm` and falls back to `find`: https://github.com/apache/trafficcontrol/blob/4aa550d319c0269f8e9fbd8fabc0aa757da1a5f9/traffic_ops_ort/traffic_ops_ort.pl#L275
   
   That's important for users who may have ATS installed somewhere other than `/opt/trafficserver`. Which is perfectly reasonable: The Linux Standard Base makes it clear `/opt` is for people who don't play well with others, standard LSB directories (`/usr/sbin`, `/etc`, `/var/log`) are preferable.
   
   I believe that's where ATS makefiles install by default, as well.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655668270


   > _"I believe that's where ATS makefiles install by default, as well."_
   
   Yes, it is, which is exactly why I ask. Thanks :+1: .


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ocket8888 merged pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c commented on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655592369


   Also, once this is merged, we should make a new Feature Issue to change Traffic Ops Delivery Service creation and modification to not create or update `location` Parameters.
   
   I thought about doing that here, but it seems safer and smaller to do them separately. And it doesn't hurt anything to keep creating them.
   
   Should be straightforward to remove. Code is here: https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/deliveryservice/deliveryservices.go#L1476


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4861: Add ORT Inferring Location Parameters

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4861:
URL: https://github.com/apache/trafficcontrol/pull/4861#issuecomment-655669673


   Should also be easy to add other package manager like `apt` in the future.
   And the `find` fallback probably works on any POSIX-like OS (though I haven't tested).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org