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

systemd create timer option #4645

Closed

Conversation

harishanand95
Copy link
Contributor

@harishanand95 harishanand95 commented Jun 28, 2016

Work left to complete :

  • Invalid input message shown for inputs.
  • Repeat Hourly, Daily, Weekly, Monthly, Yearly and dont repeat option works.
  • Tests cases

Fixes left:

@andreasn
Copy link
Contributor

andreasn commented Jul 1, 2016

Good start!
There is a couple of styling and spacing errors, but those should be quick and easy to fix.

screenshot from 2016-07-01 16-39-42

Compare with
screenshot from 2016-07-01 16-39-12

  • The spacing between the rows is too large.
  • The labels are black instead of gray.
  • The spacing between the labels on the left and the inputs on the right is too big.

@andreasn
Copy link
Contributor

andreasn commented Jul 1, 2016

Should "Service file name" be "Service name" perhaps?

@andreasn
Copy link
Contributor

andreasn commented Jul 1, 2016

Mockup for comparison.
Mockup

@andreasn
Copy link
Contributor

andreasn commented Jul 1, 2016

Instead of putting "At" on it's own row, I think it would work better to put it on the same line as the dropdown.

@harishanand95
Copy link
Contributor Author

harishanand95 commented Jul 4, 2016

Updated design changes (I think now it more follows the mock-ups) :
screenshot from 2016-07-04 16-25-37
screenshot from 2016-07-04 16-25-43
screenshot from 2016-07-04 16-25-53
screenshot from 2016-07-04 16-25-58
@andreasn in mockups, why do we have multiple number of "+" button, instead of just a single one? ( located after repeat options in mockups)

@andreasn
Copy link
Contributor

andreasn commented Jul 5, 2016

Good improvements. This is starting to shape up really well!

A couple of minor issues:

  • "Run after system boot at X seconds" doesn't sound quite right. I think "Run after system boot after X seconds" would work better.
  • It would look a bit nicer if the "at/after" label, the box and the seconds dropdown would be right aligned.
  • If the "at/after" box is left empty, does that mean the delay is zero? It would be good to fill the input with that number in that case.
  • Could the + for adding another case be on the same row as the x that removes it? That would make it consistent with how it works in the Container Run dialog.

@harishanand95 harishanand95 force-pushed the systemd-timers branch 3 times, most recently from 6183471 to fec5e2f Compare July 6, 2016 09:42
@harishanand95
Copy link
Contributor Author

harishanand95 commented Jul 6, 2016

@andreasn fixed all the issues you have mentioned.
other fixes in the commit are :

  • Create Timer button is shown in timer tab only.
  • “Invalid input” message doesnt change of position of boot_time elements.

fixes still left :

  • Repeat Yearly's datepicker menu is positioned incorrectly.
    screenshot from 2016-07-06 15-18-11

@harishanand95
Copy link
Contributor Author

@andreasn I have corrected the position of datepicker in repeat yearly option.
screenshot from 2016-07-06 17-16-31

@dperpeet
Copy link
Contributor

dperpeet commented Jul 7, 2016

"Run after system boot after X seconds" would work better.

How about "run X seconds after system boot"?

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

How about "run X seconds after system boot"?

Even though it forms a better sentence, it would make the dropdown jump around.

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

If I first create one timer, then decides to create another, the values from the old timer is still present in the timer creation dialog. It needs to be cleared out on exit.

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

The error message "within 0-59" sounds a bit odd. Maybe "Needs to be between 0-59"
Or is that too long of a string to fit?

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

Maybe this is a limitation of the calendar widget, but it feels a bit strange to be able to run a timer in the past. Not a biggie though.

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

screenshot from 2016-07-07 19-11-49

@andreasn
Copy link
Contributor

andreasn commented Jul 7, 2016

Maybe the time errors could be displayed like this instead:
screenshot from 2016-07-07 19-11-49

@harishanand95
Copy link
Contributor Author

harishanand95 commented Jul 11, 2016

@andreasn I have updated the error messages. Bootstrap datepicker is difficult to work with! I needed a hack to get its position correct. :( Give a close look at boot-strap datepickers behaviour.
Here are the screen shots:
screenshot from 2016-07-11 11-36-09
screenshot from 2016-07-11 15-08-19

@harishanand95 harishanand95 force-pushed the systemd-timers branch 2 times, most recently from ba00bca to 1d74998 Compare July 11, 2016 11:51
@andreasn
Copy link
Contributor

It seems the alignment for the labels for "Service name", "Description" and "Command" gets off once they get a error message. They get pushed too far down.

screenshot from 2016-07-11 17-08-40

Filling an entry has the opposite effect. The labels end up too far up.
screenshot from 2016-07-11 17-11-31

@andreasn
Copy link
Contributor

I wonder if it makes sense to spread the Repeat dropdown all across to line it up with the elements below better. Should be just a matter of removing the div#drop_repeat css

@harishanand95 harishanand95 force-pushed the systemd-timers branch 4 times, most recently from 0eb557c to f2d8b36 Compare July 18, 2016 09:17
@@ -194,6 +195,9 @@ WantedBy=default.target

def svc_sel(service):
return 'tr[data-goto-unit="%s"]' % service
def wait_systemctl_timer(time):
with testvm.Timeout(seconds=10, error_message="Timeout while waiting for systemctl list-timers"):
m.execute("cmd='systemctl list-timers'; until $cmd | grep -m 1 '%s'; do sleep 1; done" % time)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

m.execute("cmd='systemctl list-timers'; until $cmd | grep -m 1 '%s'; do sleep 1; done" % time) would loop and check if systemctl list-timers has the updated results. This keeps checking every second for a total of 10sec after which timeout occurs.
@dperpeet which is better sleep 1 or sleep 0.5, here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dperpeet which is better sleep 1 or sleep 0.5, here?

1 is fine

@dperpeet
Copy link
Contributor

debian tests fail - needs fix from #4857

@stefwalter stefwalter changed the title WIP systemd create timer option systemd create timer option Aug 11, 2016
return 'tr[data-goto-unit="%s"]' % service
def wait_systemctl_timer(time):
with testvm.Timeout(seconds=20, error_message="Timeout while waiting for systemctl list-timers"):
m.execute("sleep 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to sleep here? the next loops does that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought if we called systemctl list-timer in next line after 1 sec, then it can have the updated values. Let me test it locally in centos and fedora-atomic without the 1 sec and see if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests passed on centos and fedora-atomic locally.

fail(function(error) {
console.log(error);
});
console.log("#Timer file#\n" + timer_file);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dperpeet before we merge this, we should remove these console.log() lines. They give the contents of .service and .timer files.

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, I will do this during merge

@dperpeet dperpeet closed this in 5e171d1 Aug 14, 2016
@dperpeet
Copy link
Contributor

nice job, this will be part of Cockpit 0.118

@harishanand95
Copy link
Contributor Author

great! thanks everyone!

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