Enable ntp if autodetect ntpd or timesyncd#597
Enable ntp if autodetect ntpd or timesyncd#597ColinMcInnes wants to merge 5 commits intoNetworkConfiguration:masterfrom
Conversation
If --enable-ntpd or --enable-timesyncd, assume --enable-ntp and choose the appropriate hook file. Will complain if --enable-ntpd or --enable-timesyncd are both set, allows override of autodetect. Autodetect will enable ntp functionality unless explicitly told not to via --disable-ntp. If --disable-ntp is used, no time hooks are installed. Addresses NetworkConfiguration#574
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 30 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdd explicit time-service flags ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
configure (2)
1711-1728: Autodetection runs even when service is explicitly enabled.When
--enable-ntpdis passed,NTPDis alreadyyes, but the script still runs ntpd/chronyd detection. This is redundant and produces confusing output (e.g., "not found" messages even though the service was explicitly enabled).Consider skipping autodetection when the service flag is already set:
Proposed refactor
- printf "Checking for ntpd ... " - NTPD_PATH=$(_which ntpd) - if [ -n "$NTPD_PATH" ]; then - NTPD=yes - NTP=yes - echo "$NTPD_PATH (Selecting 50-ntp.conf)" - else - echo "not found" - fi + if [ "$NTPD" != "yes" ]; then + printf "Checking for ntpd ... " + NTPD_PATH=$(_which ntpd) + if [ -n "$NTPD_PATH" ]; then + NTPD=yes + NTP=yes + echo "$NTPD_PATH (Selecting 50-ntp.conf)" + else + echo "not found" + fi + fiApply similar logic to chronyd and timesyncd detection blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 1711 - 1728, The ntpd/chronyd autodetection runs unconditionally even when the service is explicitly enabled, causing confusing "not found" output; modify the detection blocks that set NTPD_PATH and CHRONYD_PATH to first check the NTPD flag (e.g., test if NTPD is empty or not "yes") and skip calling _which and the echo "not found" branch when NTPD is already set, so only run the detection logic when NTPD is unset and preserve setting NTPD=yes, NTP=yes and the "(Selecting 50-ntp.conf)" message only when a binary is actually found.
42-44:CHRONYDvariable is initialized but never used.The script uses
CHRONYD_PATHto store the detected path and setsNTPD=yeswhen chronyd is found, making theCHRONYDvariable dead code. Consider removing it or using it consistently withNTPDandTIMESYNCD.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configure` around lines 42 - 44, The CHRONYD variable is declared but never used; either remove CHRONYD entirely or make it consistent with the other toggles by setting and using it when chronyd is detected. Update the detection block that currently sets CHRONYD_PATH and NTPD=yes to also set CHRONYD=yes (or remove the CHRONYD declaration at the top and any references) and ensure any later logic that checks NTPD or TIMESYNCD uses the correct variable(s) (CHRONYD, NTPD, TIMESYNCD) consistently so there is no dead/unreferenced variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 1731-1733: The script currently echoes a warning only when both
NTPD_PATH and CHRONYD_PATH are set; add a similar warning when either NTPD_PATH
or CHRONYD_PATH is set together with TIMESYNCD_PATH so users know 50-ntp.conf
will be chosen over 50-timesyncd.conf. Locate the block referencing NTPD_PATH
and CHRONYD_PATH and insert a check like "if [ -n \"$TIMESYNCD_PATH\" ] && ([ -n
\"$NTPD_PATH\" ] || [ -n \"$CHRONYD_PATH\" ])" and echo a clear warning
indicating both an ntp implementation and timesyncd were detected and that
50-ntp.conf will be preferred over 50-timesyncd.conf.
- Around line 1706-1710: The mutual-exclusivity check for NTPD vs TIMESYNCD is
currently nested inside the HOOKSET conditional and gets skipped when
HOOKSET/--with-hooks is set; relocate the check that tests NTP, NTPD and
TIMESYNCD (the if ! [ "$NTP" = "no" ]; then ... if [ "$NTPD" = "yes" ] && [
"$TIMESYNCD" = "yes" ]; then ... exit 1 fi fi block) so it runs unconditionally
after option parsing (or immediately after the code that sets HOOKSET), ensuring
that the conflict between --enable-ntpd and --enable-timesyncd is detected even
when HOOKSET/--with-hooks is used.
---
Nitpick comments:
In `@configure`:
- Around line 1711-1728: The ntpd/chronyd autodetection runs unconditionally
even when the service is explicitly enabled, causing confusing "not found"
output; modify the detection blocks that set NTPD_PATH and CHRONYD_PATH to first
check the NTPD flag (e.g., test if NTPD is empty or not "yes") and skip calling
_which and the echo "not found" branch when NTPD is already set, so only run the
detection logic when NTPD is unset and preserve setting NTPD=yes, NTP=yes and
the "(Selecting 50-ntp.conf)" message only when a binary is actually found.
- Around line 42-44: The CHRONYD variable is declared but never used; either
remove CHRONYD entirely or make it consistent with the other toggles by setting
and using it when chronyd is detected. Update the detection block that currently
sets CHRONYD_PATH and NTPD=yes to also set CHRONYD=yes (or remove the CHRONYD
declaration at the top and any references) and ensure any later logic that
checks NTPD or TIMESYNCD uses the correct variable(s) (CHRONYD, NTPD, TIMESYNCD)
consistently so there is no dead/unreferenced variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Addressing AI review comments.
Skips autodetection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configure`:
- Around line 1713-1734: The autodetect block currently sets NTPD=yes when NTPD
is unset, which lets ntpd/chronyd detection override an explicit TIMESYNCD=yes;
modify the logic in the detection around the _which checks (ntpd and chronyd) so
they only set NTPD and NTP when TIMESYNCD is not explicitly set to yes (i.e.
guard the assignments to NTPD/NTP behind a check of TIMESYNCD and/or NTPD being
empty), and ensure the hook selection later still respects explicit TIMESYNCD by
not changing NTPD when TIMESYNCD=yes or when NTPD was explicitly set by the
user. Ensure you reference and update the detection code that uses _which, the
NTPD/NTP assignments, and the branch that chooses 50-ntp.conf so explicit
TIMESYNCD is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
enabled ntpd/chrony, timesyncd, or other HOOKSCRIPT method.
Service selection should disable autodetect of the other service.
1 participant
If --enable-ntpd or --enable-timesyncd, assume --enable-ntp and
choose the appropriate hook file.
Will complain if --enable-ntpd or --enable-timesyncd are both set,
allows override of autodetect.
Autodetect will enable ntp functionality unless explicitly told not
to via --disable-ntp. If --disable-ntp is used, no time hooks
are installed.
Addresses #574