rob05c commented on PR #6673: URL: https://github.com/apache/trafficcontrol/pull/6673#issuecomment-1099365975
` This PR has tests` `This PR has documentation` `This PR has a CHANGELOG.md entry` are all checked, but it doesn't look like any of those are in the PR. @jpappa200 mind adding them? I don't think we need docs, this behavior is incidental and the only right thing to do, we don't need to document that level of detail. But that should probably be noted, rather than checking the box that it has docs when it doesn't. But it should probably have a Changelog entry, so people know how and when it changed. And it should be straightforward to add a unit test, shouldn't need an Integration Test. In https://github.com/apache/trafficcontrol/blob/3b8c0cdc/lib/go-atscfg/parentdotconfig_test.go just needs to make a test with no Primaries and a Secondary via CacheGroups, which should pass with the code change and fail without it. Other than that, this change looks good. The previous behavior was generating broken config, this is the right thing to do. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
