Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure ntp is installed (when Install Recommended Packages is off) #55

Closed
wants to merge 8 commits into from

Conversation

mvidner
Copy link
Member

@mvidner mvidner commented Aug 4, 2015

https://bugzilla.suse.com/show_bug.cgi?id=938951

The actual fix is 13e61a9 and the rest is refactoring. But the code is spread across two packages (see a related PR in ntp-client) and I am not sure how much of a cleanup I should attempt now (and even if this is in the end a cleanup)

THROW No widget with ID `time_fr
It was removed in commit 67b1d81
Moved
 from yast2-ntp-client:clients/ntp-client_proposal.rb
 to   yast2-country:   clients/timezone_ntp.rb

because the file provides a part(!) of a dialog for the time settings,
and the interface has grown too tangled it does not make sense to
keep them in 2 packages/repos.

Yes, now the problem has been shifted a level lower,
to the NtpClient interface

Also, despite being named *_proposal, the client has never been
used in the proposal framework.

(The original file will be removed from yast2-ntp-client
once this package is released)
ntpdate_only -> oneshot (only sync now, not at boot)
ntpdate as a program to do the sync was replaced with sntp
The requirement was missing and the package got in thanks to Recommended
dependencies. These can now be turned off <https://fate.suse.com/318099>
def ntp_call(acall, args)
if !@installed
# replace "replace_point" by the widgets
if acall == "ui_init"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: I know this was simply copied from elsewhere, but using case...when here would really improve it a lot...

@ancorgs
Copy link
Contributor

ancorgs commented Aug 5, 2015

To be honest, such a big refactoring (although a big part is just code moving) without unit test scares me a bit.

@mvidner
Copy link
Member Author

mvidner commented Aug 6, 2015

@ancorgs You're right, and the API redesign is not too good either; so I am going to improve that and add tests, but proceed with the actual simple fix in another PR.

@jreidinger
Copy link
Member

@mvidner what is status here? I worry it is too risky for SP1 now, but if you improve it a bit, it can be merged for SP2

@kobliha
Copy link
Member

kobliha commented Jul 25, 2016

@mvidner SP2 is just behind the door, any progress?

@teclator
Copy link
Contributor

teclator commented Apr 4, 2017

There has not been updates since a while and the bug was already fixed by yast/yast-ntp-client#34 so maybe make sense to close it by now and just create a new PR if some improvements are done at the end.

@mvidner , what do you think about?

@mvidner mvidner closed this Apr 4, 2017
@jreidinger jreidinger deleted the ensure-ntp-installed branch August 10, 2017 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants