-
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
Support different logging paths based on OS #1102
Support different logging paths based on OS #1102
Conversation
|
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, minor comment, and there are some test failures that need to be resolved.
# (--log.frame-log-root.default_os). Command line flag will be preferred if both are provided. | ||
log.frame-log-root.default_os=${CUE_FRAME_LOG_DIR:/shots} | ||
# To set up root directories for other OS create new environment | ||
# variable as `log.frame-log-root.[OS] where OS relates to str_os on the job table |
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.
Can you add commented-out Windows and Linux examples?
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.
Just added examples:
# For example:
# log.frame-log-root.Windows=arbitraryWindowsDirectory
# log.frame-log-root.linux=arbitraryLinuxDirectory
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.
Could you expand those examples to include a (fake) path?
For example if I want to define a Windows log root, do I use forward slashes, backslashes, or is either ok? An example to work from would make it easier to know without having to figure it out for myself.
f73bdf6
to
83d221e
Compare
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 like there's a conflict with master
. Probably need a merge/rebase.
# (--log.frame-log-root.default_os). Command line flag will be preferred if both are provided. | ||
log.frame-log-root.default_os=${CUE_FRAME_LOG_DIR:/shots} | ||
# To set up root directories for other OS create new environment | ||
# variable as `log.frame-log-root.[OS] where OS relates to str_os on the job table |
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.
Could you expand those examples to include a (fake) path?
For example if I want to define a Windows log root, do I use forward slashes, backslashes, or is either ok? An example to work from would make it easier to know without having to figure it out for myself.
Link the Issue(s) this Pull Request is related to.
Opencue should support different logging paths for Windows and Linux #1096
Summarize your change.
The logging paths now can be different based on OS. The path can be set up by creating a new env variable like this:
log.frame-log-root.[OS]
where[OS]
is related tostr_os
on the job table. If no environment variable is set up, it will default to the path described incuebot/src/main/resources/opencue.properties
This change will allow OpenCue to be more adaptable to any system
The change was tested using unit tests and by running docker image.