-
Notifications
You must be signed in to change notification settings - Fork 189
CFE-2815: Added isnewerthantime
function
#5799
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
Conversation
int exit_code = stat(arg_file, &file_buf); | ||
|
||
if (exit_code == -1) | ||
{ |
Check warning
Code scanning / CodeQL
Poorly documented large function Warning
|
||
time_t file_mtime = file_buf.st_mtime; | ||
|
||
bool result = file_mtime > arg_mtime; |
Check notice
Code scanning / CodeQL
Pointer argument is dereferenced without checking for NULL Note
time_t file_mtime = file_buf.st_mtime; | ||
|
||
bool result = file_mtime > arg_mtime; | ||
|
Check notice
Code scanning / CodeQL
Pointer argument is dereferenced without checking for NULL Note
|
||
bool result = file_mtime > arg_mtime; | ||
|
||
return FnReturnContext(result); |
Check notice
Code scanning / CodeQL
Pointer argument is dereferenced without checking for NULL Note
bool result = file_mtime > arg_mtime; | ||
|
||
return FnReturnContext(result); | ||
} |
Check notice
Code scanning / CodeQL
Pointer argument is dereferenced without checking for NULL Note
|
||
return FnReturnContext(result); | ||
} | ||
|
Check notice
Code scanning / CodeQL
Pointer argument is dereferenced without checking for NULL Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Since the title will be part of the changelog you can make it a little bit more specific. E.g., "Added isnewerthantime() policy function". Also backticks in the changelog can cause som issues later on. So better omit them in the case of changelog entries.
It would also be nice if you included an acceptance test. Here you can add more test cases that are not suitable in an example. E.g., what happens if the timestamp is not a string, what happens in the timestamp is CF_NOINT
, what happens if timestamp is CF_INFINITY
etc.
You will also have to sign the CLA #5799 (comment) |
Changelog: None Signed-off-by: jakub-nt <[email protected]>
Ticket: CFE-2815 Changelog: Title Signed-off-by: jakub-nt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
if (exit_code == -1) | ||
{ | ||
return FnFailure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could add some user friendly error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general it's a good idea to add helpful error messages, but looking at the other similar policy functions below, this seems consistent with them - they also return FnFailure()
without printing an error, when stat
fails, indicating:
- Maybe it's desirable to not print anything? Would adding errors become too noisy?
- Maybe it's intentional, since something else is responsible for printing the error automatically when you return
FnFailure()
? - Maybe it is really a mistake, and we should update all of these to have error messages?
I haven't looked into it further, so I don't know the answer. If you find out, and it's 1. or 2., probably a comment would be nice instead, or a debug level message, or both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 🚀
inputs => { "../../default.cf.sub" }; | ||
bundlesequence => { default("$(this.promise_filename)") }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are not using anything from default.cf.sub you can consider not including it and instead write your own bundle sequence.
inputs => { "../../default.cf.sub" }; | |
bundlesequence => { default("$(this.promise_filename)") }; | |
bundlesequence => { "check" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you can omit body common control and just have bundle agent main and then use methods promises to call each of the other bundles you want.
bundle agent __main__
{
methods:
"init";
"test";
"check";
"cleanup";
So, many ways to skin the cat.
{ | ||
vars: | ||
"test_file" | ||
string => "$(this.promise_dirname)/isnewerthantime_file.txt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can consider creating this file in an init
bundle and setting the mtime
, all using the files
promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation is using the derived mtime via filestat()
corresponding docs PR: cfengine/documentation#3430