remove ntp from core #20623
remove ntp from core #20623
Conversation
kbrock
requested review from
agrare,
Fryguy,
gtanzillo and
jrafanie
as code owners
locale/en/manageiq.po
Outdated
| @@ -32333,10 +32333,6 @@ msgstr "" | |||
| msgid "Attach Volume" | |||
| msgstr "" | |||
|
|
|||
| #: ../app/views/cloud_volume/attach.html.haml:25 | |||
| msgid "Device Mountpoint" | |||
There was a problem hiding this comment.
@kbrock got the ✂️ in a groove and did a little too much.
✂️ 🔥 ✂️ 🔥 ✂️ 🔥
🍰 🍪 👏 🙇 😍 🎉
There was a problem hiding this comment.
Mountpoint has ntp buried in it. Hence the overzealous ✂️ ✂️ ✂️ ✂️
There was a problem hiding this comment.
Other than the extra lines deleted pointed out by Brandon, I think this greatly depends on the results of the discussion in ManageIQ/manageiq-api#892 or that discussion should move here.
locale/en/manageiq.po
Outdated
| @@ -32333,10 +32333,6 @@ msgstr "" | |||
| msgid "Attach Volume" | |||
| msgstr "" | |||
|
|
|||
| #: ../app/views/cloud_volume/attach.html.haml:25 | |||
| msgid "Device Mountpoint" | |||
There was a problem hiding this comment.
@kbrock got the ✂️ in a groove and did a little too much.
✂️ 🔥 ✂️ 🔥 ✂️ 🔥
🍰 🍪 👏 🙇 😍 🎉
| @@ -11,7 +11,6 @@ | |||
| "Example (Boolean): #{__FILE__} -s 1 -p ui/mark_translated_strings -v true -t boolean\n" \ | |||
| "Example (Symbol): #{__FILE__} -s 1 -p workers/worker_base/queue_worker_base/ems_metrics_collector_worker/defaults/poll_method -v escalate -t symbol\n" \ | |||
| "Example (Float): #{__FILE__} -s 1 -p capacity/profile/1/vcpu_commitment_ratio -v 1.5 -t float" \ | |||
| "Example (Array): #{__FILE__} -s 1 -p ntp/server -v 0.pool.ntp.org,1.pool.ntp.org -t array" | |||
There was a problem hiding this comment.
ooh, you found that one too... nice
| # dozens of calls to `systemctl restart chronyd` simultaneously. | ||
| # Instead, this method will allow it to only happen once on | ||
| # the reload of settings in evmserverd. | ||
| private def activate_settings_for_appliance |
ntp settings can be updated at configuration time along with timezone and current time. makes more sense to consolidate these in one place. Since all 3 are so necessary, they are available from the cloud/infra configuration scripts and ui
kbrock
force-pushed
the
no_time_like_now
branch
from
f879c66 to
50c6272
Compare
|
Checked commit kbrock@50c6272 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint |
|
ok, fine. added mount points back. made my numbers worse but guess you gotta do what you gotta do. |
|
cross repo: ManageIQ/manageiq-cross_repo-tests#182 let me know if you want me to hit more repos with that |
|
Ugh - don't know what my problem was but now cross repo tests are passing and this is good to go |
There was a problem hiding this comment.
Based on the discussions, it seems like everyone agrees that configuring NTP servers is something that does not belong in the application. (And it's also been removed from the UI in ManageIQ/manageiq-ui-classic#7369)
|
@kbrock Do you think we need a DB migration to remove any |
We took time and timezone out of our app because it makes more sense to configure those parameters at the time of deployment. Tools like cockpit do that configuration for us. appliance console 99
Those configuration screens, also configure ntp server. Since ntp server is time related, this makes sense. The pr referenced above even has a comment that says we should remove this attribute next.
This PR removes ntp configuration from the core. It also needs to be removed from the ui.
Also will remove it from the ui.
We are removing ntp server here because it either needs to be add to the api, or removed from the system.
We need to make a decision so we can move forward with ManageIQ/manageiq-api#892
/cc @bdunne @skateman