-
Notifications
You must be signed in to change notification settings - Fork 3
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 timestamp on backup files #52
Conversation
Signed-off-by: lbgracioso <[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.
I think this idea is very good, but I would suggest to stick with standards like ISO 8601 with is supported directly on the language.
I've added comments directly on the code to address those issues.
Hi @lbgracioso did you took a chance to look at the comments on this one? |
Signed-off-by: Lucas Gracioso <[email protected]>
@viniciusferrao Can you please check it again? |
* \brief Get the current timestamp as a string. | ||
* | ||
* This function generates the current timestamp in the format | ||
* "YYYYMMDD_HHMMSS". |
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.
Docs needs to be also updated.
time_t now; | ||
time(&now); | ||
char buf[sizeof "2011-10-08T07:07:09Z"]; | ||
strftime(buf, sizeof buf, "%FT%TZ", gmtime(&now)); |
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.
Why not use chrono
instead?
strftime
is a C directive that's available on ctime
and we should definitely avoid using legacy C functions.
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.
Yes, this can be done with chrono
https://en.cppreference.com/w/cpp/chrono
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.
Need to use stringstream if using chrono. Already discussed with @viniciusferrao
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.
The beautiful thing of C++ is the legacy that we have. There's probably a way to completely ditch stringstream
because it sucks and we have std::format
now, however we just need to find a way to use it with chrono
.
Dropping it because #64 is already merged. |
I've merged wrong @arthurmco? |
This PR adds the current timestamp (ISO 8601 like) to the end of the backup file names
Example: Created a backup copy of /etc/exports on /opt/cloysterhpc/backup/etc/exports_2024-06-19T11:08:45Z