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.

Sign up for GitHub

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

Jump to bottom

Log last cron execution #7689

Merged
merged 5 commits into from
Mar 25, 2014
Merged

Log last cron execution #7689

merged 5 commits into from
Mar 25, 2014

Conversation

Niduroki
Copy link
Member

Fixes #2012

@karlitschek @DeepDiver1975
@jancborchardt have a look at the design/feel free to improve this yourself. atm this is rather ugly.

We may also remove the squircles from files_external's css, as I put these into settings's css.

@ghost
Copy link

ghost commented Mar 12, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3694/

@karlitschek
Copy link
Contributor

looks good. not tested 👍

@jancborchardt
Copy link
Member

@Kondou-ger can you supply some screenshots?

@Niduroki
Copy link
Member Author

@jancborchardt
screenshot491

@jancborchardt
Copy link
Member

Can you put it directly right next to the header? Otherwise it looks like it’s another radiobutton. :)

@Niduroki
Copy link
Member Author

@jancborchardt still looks a bit weird …
screenshot492

DeepDiver1975
DeepDiver1975 reviewed Mar 12, 2014
View reviewed changes
settings/templates/admin.php
<?php if ($_['cron_log']): ?>
<p class="cronlog inlineblock">
<?php if ($_['lastcron'] !== false):
$human_time = date('Y-m-d H:i', $_['lastcron']) . " UTC";
Copy link
Member

Choose a reason for hiding this comment

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

oc_util or oc_helper has some date time formating function - please use that

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 prefer ISO 8601, but okay.

DeepDiver1975
DeepDiver1975 reviewed Mar 12, 2014
View reviewed changes
settings/templates/admin.php
<?php if ($_['cron_log']): ?>
<p class="cronlog inlineblock">
<?php if ($_['lastcron'] !== false):
$human_time = OC_Util::formatDate($_['lastcron']) . " UTC";
Copy link
Member

Choose a reason for hiding this comment

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

Is the result of formatDate UTC?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because the input is UTC, because time() outputs UTC.

@jancborchardt
Copy link
Member

@Kondou-ger I think it looks good. Cron, and then directly the status of it.

@ghost
Copy link

ghost commented Mar 12, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3702/

@PVince81
Copy link
Contributor

Nice feature!

Two points:

  1. The name "cron_log" is a bit confusing. I expected it to add an entry in the log, not just save the last date. It seems this is not really about logging anything, but just remembering the last cron value.
  2. Do we need the "cron_log" setting ? Is is a performance or security issue to always save the last run date in the database ? I'd prefer to go for simplicity and have this always enabled, and no config setting. (which would also solve point 1))

I will test this once these questions have been addressed.

@Niduroki
Copy link
Member Author

@PVince81

  1. This feature logs the last successfull cron execution, if you have a better name: Go for it, tell me 😄
  2. Everytime cron.php is called there's a db access. This may be 1. db heavy if you're using ajax-cron with many users 2. useless if you know cron is working properly. Thus I added this option.

@PVince81
Copy link
Contributor

@Kondou-ger I was trying to find a better name but now my brain is stuck on "log last cron execution"... so let's keep it.

Point 2 makes sense.

Let me test this.

@PVince81
Copy link
Contributor

Seems to work fine, except that even though the "status" element is here, I don't see a green dot inside. Note that to test this I used the merge instructions, so it is possible that some change on master breaks it.
Maybe you can rebase it onto master, then you'll see the issue.

@Niduroki
Copy link
Member Author

Somehow I forgot to move the squircles from files_external to core so they only show when you have to files_external app active.

@scrutinizer-notifier
Copy link

The inspection completed: 3 new issues

@ghost
Copy link

ghost commented Mar 21, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3841/

@tomneedham
Copy link
Member

👍 Looks nice and seems to function as intended.

@PVince81
Copy link
Contributor

I can see the circle now 👍

PVince81 pushed a commit that referenced this pull request Mar 25, 2014
Merge pull request #7689 from owncloud/last_cron_log
5111fad 
Log last cron execution
@PVince81 PVince81 merged commit 5111fad into master Mar 25, 2014
@DeepDiver1975 DeepDiver1975 mentioned this pull request Mar 26, 2014
Closed
@jancborchardt jancborchardt deleted the last_cron_log branch June 5, 2014 19:00
MorrisJobke added a commit to nextcloud/server that referenced this pull request Aug 16, 2017
@MorrisJobke
Always log cron execution
424f671 
There was a setting to disable the last execution of cron. There is no known
problem with this write access and it was also questioned when this feature
was build in owncloud/core#7689 (comment)

Recently there was also a bug report about a non-visible last cron execution
(#6088) - let's better remove this.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke mentioned this pull request Aug 16, 2017
Merged
MorrisJobke added a commit to nextcloud/server that referenced this pull request Aug 17, 2017
@MorrisJobke
Always log cron execution
1c38a83 
There was a setting to disable the last execution of cron. There is no known
problem with this write access and it was also questioned when this feature
was build in owncloud/core#7689 (comment)

Recently there was also a bug report about a non-visible last cron execution
(#6088) - let's better remove this.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@lock lock bot locked as resolved and limited conversation to collaborators Aug 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement settings:admin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: Status indicator for cron-system
7 participants
@Niduroki @karlitschek @jancborchardt @PVince81 @scrutinizer-notifier @tomneedham @DeepDiver1975