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.

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

Convert log paths between OSs #1138

Merged

Conversation

DiegoTavares
Copy link
Collaborator

As windows job logs are now having their path saved to the database (#1133), the
GUI needs to be able to convert paths so that multiple OSs can read the
logs using the GUI.

As windows job logs are now having their path saved to the database, the
GUI needs to be able to convert paths so that multiple OSs can read the
logs using the GUI
@DiegoTavares
Copy link
Collaborator Author

@bcipriano would you review this?

Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

LGTM. We need a way to make those roots configurable without changing the code but no need to do it here.

if my_os != job_os and \
my_os in cuegui.Constants.LOG_ROOT_OS and \
job_os in cuegui.Constants.LOG_ROOT_OS:
log_dir = log_dir.replace(cuegui.Constants.LOG_ROOT_OS[job_os],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually it strikes me that replace might cause some unintended changes to the path, depending on what folks are using as their roots. Maybe we could do the substitution using trim?

LOG_ROOT_OS = {
"rhel7": "/shots",
"linux": "/shots",
"windows": "S:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be nice to have a macOS example in here as well.

cuegui/cuegui/Utils.py Outdated Show resolved Hide resolved
Still using replace, but only targeting the first occurrence to avoid unintended substitutions

Added a macos example
@Eiken
Copy link

Eiken commented May 25, 2022

So great to see work on this. Would it also be possible to separate the root blades write command logs to from where cuegui reads the logs? In our current farm setup we write logs locally and use filebeat and logstash to get logs to the central location to avoid nfs mounts when scaling to cloud.

@DiegoTavares
Copy link
Collaborator Author

@bcipriano your suggestions have been applied. I'm still using replace, but only targeting the first occurrence to avoid unintended substitutions as suggested.

@DiegoTavares
Copy link
Collaborator Author

So great to see work on this. Would it also be possible to separate the root blades write command logs to from where cuegui reads the logs? In our current farm setup we write logs locally and use filebeat and logstash to get logs to the central location to avoid nfs mounts when scaling to cloud.

That makes total sense. Would you mind adding a new feature request for this?

@DiegoTavares DiegoTavares merged commit 4402c1a into AcademySoftwareFoundation:master May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants