-
Notifications
You must be signed in to change notification settings - Fork 201
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 better logging functionality #119
Conversation
What I personally don't like is that messages from scripts are a bit harder to stop. Some "ascii art" could help, e.g. |
@@ -0,0 +1,3 @@ | |||
function info-log { |
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'm not much a fan of dashes in function name, something like log_info
(words deliberately swapped) would create space for potential log_warning
, log_error
in the future.
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.
Btw. I'm not seeing the reason to have a separate file and not define the function directly in common.sh
, but you might have a reason..
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 second renaming this to log_info.
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.
and putting it in common.sh
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 reason why I wanted it outside of common.sh is that common.sh isn't really common across images. I wanted to have a separate file that could be directly copyable so it would be easy to keep up-to-date.
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'm ok with renaming.
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 reason why I wanted it outside of common.sh is that common.sh isn't really common across images. I wanted to have a separate file that could be directly copyable so it would be easy to keep up-to-date.
so "morecommon.sh"? :) fair enough i guess.
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.
common_functions.sh
?
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.
common_functions.sh
or morecommon.sh
are just weird. I like lib.sh
best, because it kind of is a "library".
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.
let's not dwell on this. lib.sh is fine. my "morecommon.sh" suggestion was a joke, if that wasn't clear.
Generally I like this and agree we should play with it a bit... What about UTF instead of ASCII, like prefixing with |
Having the container id in the output feels like overkill to me. Maybe worth having if debug mode is enabled, but otherwise it's pretty ugly and rarely likely to be all that useful. i think we should just copy the assemble syntax we've used for delineating our log entries: |
@bparees it's actually $HOSTNAME. I don't know, I thought it might be useful. |
@mnagy i'm even less inclined to keep it if it's $HOSTNAME :) |
77deeb0
to
09c73e9
Compare
I don't have a big problem with |
. ${CONTAINER_SCRIPTS_PATH}/passwd-change.sh | ||
fi | ||
if [ -f ${CONTAINER_SCRIPTS_PATH}/post-init.sh ]; then | ||
log_info 'Sourcing post-init.sh ...' |
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.
What does this message mean for the user? Wouldn't be better to explain what post-init do here?
09c73e9
to
569a31c
Compare
Addressed Michal's comments. PTAL |
[test][openshift] |
LGTM |
lgtm |
Add better logging functionality
This PR has different patches for the 5.5 dir and 5.6 dir. I made a diff of the diffs: Much easier to visualize with a tool like In summary:
I'm trying to understand the current state of the replication examples. Thanks! |
This is my first stab at #112. I'd like to get feedback before adopting this for 5.6 and other DB images. For now I'm only adding one logging function (first point of #112), but will add debugging variable shortly.
CC: @bparees @rhcarvalho @hhorak @praiskup @eliskasl @mfojtik
Sample run: