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

Add a NtpSetup installation dialog #791

Merged
merged 8 commits into from
Apr 3, 2019
Merged

Add a NtpSetup installation dialog #791

merged 8 commits into from
Apr 3, 2019

Conversation

dgdavid
Copy link
Member

@dgdavid dgdavid commented Mar 29, 2019

Problem

The screen for configuring NTP, currently available at yast2-caasp, should be able to be used for whatever role.

Short description of the original problem.

Solution

Move the dialog to yast2-installation.

Testing

  • Also moved the unit test available in yast2-caasp
  • Tested manually in TW and Kubic

Screenshots

  • TW
NTP Setup Validation
Screenshot_test_2019-03-28_22:52:27 Screenshot_test_2019-03-28_22:52:45
  • KUBIC
NTP Setup Validation Help
Screenshot_test_2019-03-29_10:47:27 Screenshot_test_2019-03-29_10:48:05 Screenshot_test_2019-03-29_10:47:50

@dgdavid dgdavid force-pushed the feature/ntp-setup branch 3 times, most recently from 93dd007 to 40eb413 Compare March 29, 2019 12:31
src/lib/installation/dialogs/ntp_setup.rb Outdated Show resolved Hide resolved
#
# @return [Array<String>] proposed NTP servers, empty if nothing suitable found
def ntp_servers
# TODO: use Yast::NtpClient.ntp_conf if configured
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand this TODO comment. I mean, it is not clear to me if we can do what it is stated there or simply keep the current behavior and also de TODO comment.

@lslezak it seems that you wrote it originally. Any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC that value is set in the time zone configuration (if you go to the details and configure the NTP server there), so there a small duplication.

For now I'd just move it without further changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, keep it as it is, right?

src/lib/installation/dialogs/ntp_setup.rb Show resolved Hide resolved
@dgdavid dgdavid marked this pull request as ready for review March 29, 2019 13:01
@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage increased (+0.4%) to 23.686% when pulling f90b25e on feature/ntp-setup into 7df799d on master.

Dockerfile Outdated Show resolved Hide resolved
@dgdavid dgdavid force-pushed the feature/ntp-setup branch 2 times, most recently from b158835 to d79e249 Compare April 1, 2019 11:26
Co-Authored-By: David Díaz González <dgonzalez@suse.de>
@dgdavid dgdavid merged commit fe848cc into master Apr 3, 2019
@yast-bot
Copy link
Contributor

yast-bot commented Apr 3, 2019

✔️ Public Jenkins job #53 successfully finished
✔️ Created OBS submit request #690980

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.

None yet

5 participants