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

Do not log into zowe.runtimeDirectory #726

Merged
merged 5 commits into from
Aug 28, 2024
Merged

Do not log into zowe.runtimeDirectory #726

merged 5 commits into from
Aug 28, 2024

Conversation

Martin-Zeithaml
Copy link
Contributor

@Martin-Zeithaml Martin-Zeithaml commented Aug 20, 2024

Proposed changes

This PR addresses Issue: zowe/zowe-install-packaging#3495

This PR depends upon the following PRs:

Type of change

  • Refactor the code
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

  • If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
  • My code follows the style guidelines of this project (see: Contributing guideline)
  • Relevant update to CHANGELOG.md
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

zowe.logDirectory not used in config:

No log directory. Logging disabled.     
ZWED_NODE_LOG_FILE=/dev/null            
...
No log directory. Logging disabled.    
ZWES_LOG_FILE=/dev/null                

zowe.logDirectory set to /zowe/logs/

ZWED_NODE_LOG_FILE=/zowe/logs/appServer-2024-08-27-07-39.log  
ZWES_LOG_FILE=/zowe/logs/zssServer-2024-08-27-07-39.log   

Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>
@1000TurquoisePogs
Copy link
Member

You say "ommiting zowe.logDirectory forces the zss to log into the zowe.runtimeDirectory"
But that was by design.
It's intended for development: if you don't have a log directory, where should we log? We decided to log in the same location.

So yes you can change this behavior, but it sounds like the bug to me is actually: why does a user not have zowe.logDirectory ?
Sounds like something the schema should prevent?

@Martin-Zeithaml
Copy link
Contributor Author

Martin-Zeithaml commented Aug 21, 2024

  1. The zowe.logDirectory is not mandatory and it is not in defaults.yaml. If you simply delete/comment out logDirectory line from config, it is valid from the schema point of view and app-server and zss are logging into zowe.runtimeDirectory.
  2. I have created this issue to warn users from using zowe.runtimeDirectory.
    a) If this check will be implemented, we might check (just for sure) all directories. It is possible, some new users locate certificates into zowe.runtimeDirectory.
  3. Making zowe.logDirectory mandatory in schema validation is too aggressive - you must have logDirectory for every zwe command.
  4. I don't know, if schema is capable of such complicated check, what we really need is to compare "os.realpath(*Directory) vs os.realpath(runtimeDirectory)" - you can define there symbolic links, relative/absolute path and who knows what else.
  5. At this point, you can define zowe.logDirectory and later comment it out - this check is better to do during the startup

Possible solution in launcher?

We make it the same as for zowe.workspaceDirectory:

  • zowe.workspaceDirectory is not mandatory, but you cannot run zowe without it:
ZWESVUSR INFO ZWEL0069I Configuration is valid             
ZWESVUSR ERROR ZWEL0059E failed to get WORKSPACE_DIR dir   
  • We could possibly set /dev/null as default
    • By default, the logging is off, if you want to log, specify the directory.
    • Edit: Internal start prepare is trying to create /dev/null and fails, this should be already checked, since setting zowe.logDirectory=/dev/null is allowed
  • Launcher could do the realpath check and either issue warning or stop if the extensionDirectory | workspaceDirectory | logDirectory is nested in runtimeDirectory
  • Following setting will be illegal/not recommended:
  logDirectory: ${{ zowe.runtimeDirectory }}/logs
  workspaceDirectory: ${{ zowe.runtimeDirectory }}/workspace
  extensionDirectory: ${{ zowe.runtimeDirectory }}/extension

@Martin-Zeithaml Martin-Zeithaml changed the title Do not log into zowe.runtimeDirectory Do not log into zowe.runtimeDirectory [WIP] Aug 21, 2024
Martin-Zeithaml and others added 4 commits August 23, 2024 10:59
Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>
Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>
Signed-off-by: Martin Zeithaml <Martin.Zeithaml@broadcom.com>
Signed-off-by: Martin Zeithaml <66114686+Martin-Zeithaml@users.noreply.github.com>
@Martin-Zeithaml Martin-Zeithaml changed the title Do not log into zowe.runtimeDirectory [WIP] Do not log into zowe.runtimeDirectory Aug 27, 2024
Copy link
Contributor

@JoeNemo JoeNemo left a comment

Choose a reason for hiding this comment

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

Approved.

@JoeNemo JoeNemo merged commit 144625d into v3.x/staging Aug 28, 2024
5 checks passed
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.

3 participants