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/10/05 20:02:02 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5101: Miscellaneous fixes
ocket8888 opened a new pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue
This PR fixes some experimental/miscellaneous Go tools and/or their tests that had compilation issues. They were:
- experimental TR in Go
- test/router's DNSSEC test suite
- the `traffic_vault_util` program under `tools/`
- the semi-experimental `t3c` tool in `traffic_ops_ort/`
The size of this PR is nearly exclusively due to moving vendored packages from `traffic_ops/vendor` into `vendor/` so that `traffic_vault_util` would compile. They might look like big changes, but the files were just renamed without changes and the Go tests/CiaB Action should demonstrate this had no effect on compiling Traffic Ops.
## Which Traffic Control components are affected by this PR?
- Traffic Ops ORT (t3c thing)
## What is the best way to verify this PR?
The checks should be able to do the heavy lifting, but you could also check each tool I listed above and make sure it compiles/passes its own tests/ whatever else. Or don't; they're largely experimental.
## If this is a bug fix, what versions of Traffic Control are affected?
- master
## The following criteria are ALL met by this PR
- [x] This PR fixes existing tests
- [x] Documentation is unnecessary for bug fixes
- [x] An update to CHANGELOG.md is not necessary for changes to experimental things.
- [x] This PR includes any and all required license headers
- [x] This PR does not include a database migration
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
----------------------------------------------------------------
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] zrhoffman commented on a change in pull request #5101: Miscellaneous fixes
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101#discussion_r506751934
##########
File path: test/router/dnssec/dnssec_suite_test.go
##########
@@ -20,14 +20,14 @@ package dnssec_test
*/
import (
- . "github.com/onsi/ginkgo"
- . "github.com/onsi/gomega"
+ // _ "github.com/onsi/ginkgo"
+ // _ "github.com/onsi/gomega"
Review comment:
These can just be removed, right?
##########
File path: test/router/dnssec/dnssec_suite_test.go
##########
@@ -39,17 +39,32 @@ func init() {
flag.StringVar(&deliveryService, "ds", "changeit", "ds is used to target some dns DS and DNS queries made by traffic router")
}
-var _ = BeforeSuite(func() {
+// var _ = BeforeSuite(func() {
+// d = &dnssec.DnssecClient{new(dns.Client)}
+// d.Net = "udp"
+
+// Expect(nameserver).ToNot(Equal("changeit"), "Pass in a ns flag with the hostname of the traffic router")
+// Expect(deliveryService).ToNot(Equal("changeit"), "Pass in a ds flag with the dns label for a DNS delivery service")
+// log.Println("Nameserver", nameserver)
+// log.Println("DeliveryService", deliveryService)
+// })
+
+// func TestDnssec(t *testing.T) {
+// RegisterFailHandler(Fail)
+// RunSpecs(t, "Dnssec Suite")
+// }
Review comment:
Can these commented lines be removed?
----------------------------------------------------------------
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 #5101: Miscellaneous fixes
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101#discussion_r508880602
##########
File path: test/router/dnssec/dnssec_suite_test.go
##########
@@ -20,14 +20,14 @@ package dnssec_test
*/
import (
- . "github.com/onsi/ginkgo"
- . "github.com/onsi/gomega"
+ // _ "github.com/onsi/ginkgo"
+ // _ "github.com/onsi/gomega"
Review comment:
Actually, based on what I said below, I think I should change those back to `.` imports. `.` and `_` don't do the same thing for imports, which is what I thought when I changed that. When you use `.` it imports all symbols from the package into the current namespace, which might be necessary for the code below to compile (although it actually didn't compile). So if I'm leaving in the below code, I'll wanna fix this to just be commented-out and not also changed.
----------------------------------------------------------------
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] zrhoffman commented on a change in pull request #5101: Miscellaneous fixes
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101#discussion_r509759851
##########
File path: test/router/dnssec/dnssec_suite_test.go
##########
@@ -39,17 +39,32 @@ func init() {
flag.StringVar(&deliveryService, "ds", "changeit", "ds is used to target some dns DS and DNS queries made by traffic router")
}
-var _ = BeforeSuite(func() {
+// var _ = BeforeSuite(func() {
+// d = &dnssec.DnssecClient{new(dns.Client)}
+// d.Net = "udp"
+
+// Expect(nameserver).ToNot(Equal("changeit"), "Pass in a ns flag with the hostname of the traffic router")
+// Expect(deliveryService).ToNot(Equal("changeit"), "Pass in a ds flag with the dns label for a DNS delivery service")
+// log.Println("Nameserver", nameserver)
+// log.Println("DeliveryService", deliveryService)
+// })
+
+// func TestDnssec(t *testing.T) {
+// RegisterFailHandler(Fail)
+// RunSpecs(t, "Dnssec Suite")
+// }
Review comment:
Sounds good to me
----------------------------------------------------------------
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 #5101: Miscellaneous fixes
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101#discussion_r508878761
##########
File path: test/router/dnssec/dnssec_suite_test.go
##########
@@ -39,17 +39,32 @@ func init() {
flag.StringVar(&deliveryService, "ds", "changeit", "ds is used to target some dns DS and DNS queries made by traffic router")
}
-var _ = BeforeSuite(func() {
+// var _ = BeforeSuite(func() {
+// d = &dnssec.DnssecClient{new(dns.Client)}
+// d.Net = "udp"
+
+// Expect(nameserver).ToNot(Equal("changeit"), "Pass in a ns flag with the hostname of the traffic router")
+// Expect(deliveryService).ToNot(Equal("changeit"), "Pass in a ds flag with the dns label for a DNS delivery service")
+// log.Println("Nameserver", nameserver)
+// log.Println("DeliveryService", deliveryService)
+// })
+
+// func TestDnssec(t *testing.T) {
+// RegisterFailHandler(Fail)
+// RunSpecs(t, "Dnssec Suite")
+// }
Review comment:
Maybe. I'm not totally convinced that my new code is doing exactly the same thing as that old code is meant to do. But I am sure it compiles now. Since I'm not sure what the intention was, and I don't feel the need to make a test for something experimental very clean, I'm inclined to leave it in.
----------------------------------------------------------------
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] zrhoffman merged pull request #5101: Miscellaneous fixes
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5101:
URL: https://github.com/apache/trafficcontrol/pull/5101
----------------------------------------------------------------
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