Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

[Issue-1617] - Traffic Ops Keeps track of configuration differences between database and Traffic Servers#1993

Open
KevinMackenzie wants to merge 4 commits intoapache:masterfrom
KevinMackenzie:issue-1617
Open

[Issue-1617] - Traffic Ops Keeps track of configuration differences between database and Traffic Servers#1993
KevinMackenzie wants to merge 4 commits intoapache:masterfrom
KevinMackenzie:issue-1617

Conversation

@KevinMackenzie
Copy link
Copy Markdown
Contributor

PR for feature #1617

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Mar 12, 2018

Can one of the admins verify this patch?

1 similar comment
@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Mar 12, 2018

Can one of the admins verify this patch?

Copy link
Copy Markdown
Member

@dangogh dangogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pointed out a few things based on changes that we've made recently in the go implementation. Good start, though..

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing Apache license header

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason this is not just id?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no functional reason

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every other table uses id -- should stick with that...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, can do

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these PrivLevel consts should go away -- we're using the auth.PrivLevel... consts to control individual routes in routes.go

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this Response struct used in more than one place? If not, should probably just define it where it's used rather than making it exported.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this also include domainName to make sure it's unique?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I didn't know whether host_names were unique or not. Should the host_name and domain_name be sufficient?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably just use QueryRow() which will return a single response

@dangogh
Copy link
Copy Markdown
Member

dangogh commented Mar 12, 2018

ok to test

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Mar 12, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1229/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Mar 17, 2018

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1251/
Test PASSed.

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Jun 20, 2018

Can one of the admins verify this patch?

@anna-k-1
Copy link
Copy Markdown
Contributor

@KevinMackenzie are you still interested in getting this merged?

@KevinMackenzie
Copy link
Copy Markdown
Contributor Author

Yea, I can update the code to fit the current structure and guidelines if the feature still makes sense for the current state of the project (this PR is from when trafficcontrol still had incubator status).

@mitchell852 mitchell852 added Traffic Ops related to Traffic Ops Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script new feature A new feature, capability or behavior labels Oct 24, 2019
@mitchell852 mitchell852 added cache-config Cache config generation and removed Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script labels Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cache-config Cache config generation new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants