Add servers under *.time.aws.com and move all *.pool.ntp.org servers to their own section. - #37
Add servers under *.time.aws.com and move all *.pool.ntp.org servers to their own section.#37bknowles wants to merge 2 commits into
Conversation
Add more AWS time servers, move all the *.pool.ntp.org time servers to their own section.
Fix DNS names for *.time.aws.com.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 1e0f6fa in 3 minutes and 47 seconds. Click for details.
- Reviewed
36lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. README.md:41
- Draft comment:
Ensure the updated stratum (4) for the AWS servers (0.time.aws.com, etc.) is intentional, since the removed amazon.pool.ntp.org entries had different stratum values (1/2). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a "verify that..." type comment asking the author to confirm their intention. The rules explicitly state not to make such comments. Additionally, the change appears intentional since it's consistently applied across all AWS entries. The author likely has a good reason for this change and doesn't need to verify it. The stratum level change from 1/2 to 4 is significant and could impact time synchronization quality. Maybe this deserves attention? While the stratum change is notable, asking for verification doesn't add value. If the author made a mistake, they should catch it in their own testing. The rules clearly state not to ask authors to verify their changes. Delete this comment as it violates the rule against asking authors to verify their changes. Trust that the author intentionally made this change.
2. README.md:47
- Draft comment:
Consider adding a header or clearer visual separator for the new pool.ntp.org section to improve table readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. README.md:56
- Draft comment:
Typo: Please check the spelling of "Ubiquiti Unifi". The product is typically spelled as "UniFi". Consider correcting this if appropriate. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While "UniFi" is technically the correct brand spelling, this is a minor cosmetic issue. The hostname ubnt.pool.ntp.org will work regardless of how we capitalize UniFi in the description. The comment doesn't point out any functional issues or needed code changes. It's purely about documentation formatting. The brand name spelling could matter for consistency and professionalism. Official documentation should use correct product names. While brand consistency is good, this is a minor documentation issue that doesn't affect functionality. The rules state not to make purely informative comments. Delete this comment as it's a minor documentation issue that doesn't require any functional changes.
Workflow ID: wflow_EeNpgPZeQkIUqRcn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
I think it would be counter productive to include all of the *.pool.ntp.org entries but I am not opposed to including some of the big/vendor providers. @bknowles Please redo your PR by adding the entries into ntp-sources.yml and then running the script per instructions. Thanks. |
Note that I did not add or change any *.pool.ntp.org entries. I simply moved around the ones that were already present, and put them into a separate section. If you want to put them all into a separate file, I'm not opposed to that, but I would think that would be more a personal choice on your end as to where you want to put the entries that were already present in your document. The only entries I added were for the NTP Time Servers that Amazon/AWS operate themselves, for the benefit of their customers. |
I worked on the team at AWS that ran all the NTP time services for all of Amazon and AWS, and I also helped Ask Bjørn Hansen set up and administer the NTP Pool project at
pool.ntp.org(way back in the day).Important
Adds AWS time servers and reorganizes NTP pool servers in
README.md.0.time.aws.com,1.time.aws.com,2.time.aws.com,3.time.aws.comunder Amazon NTP section inREADME.md.*.pool.ntp.orgservers to a new section labeled "NTP global public Pool" inREADME.md.README.md.This description was created by
for 1e0f6fa. You can customize this summary. It will automatically update as commits are pushed.