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

Fixed DictFile.append(), Glidein logging, and added logserver #467

Conversation

mambelli
Copy link
Contributor

Fixed and disabled DictFile.append(), replaced by add_environment()

append was used only for environment variables in the condor submit file and was causing a malformed submit file.
add_environment was already used at times and is a better method. append was fixed and is still there commented in case we'd like to use it for other attributes in the future.

@mambelli mambelli self-assigned this Dec 16, 2024
@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch from af95000 to 83a5f5c Compare December 16, 2024 07:33
factory/glideFactory.py Fixed Show fixed Hide fixed
@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch 9 times, most recently from d8181ec to 91cfd5f Compare December 18, 2024 06:16
@mambelli mambelli marked this pull request as ready for review December 18, 2024 16:05
This was referenced Dec 18, 2024
@namrathaurs namrathaurs self-requested a review December 19, 2024 20:18
append was used only for environment variables in the condor submit file and was causing a malformed submit file

add_environment was already used at times and is a better method. append was fixed and is still there commented in case we'd like to use it for other attributes in the future
@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch from 91cfd5f to 2b33bc0 Compare December 19, 2024 20:52
Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Some minor comments require to be addressed.

CHANGELOG.md Outdated Show resolved Hide resolved
logserver/web-area/put.php Outdated Show resolved Hide resolved
creation/web_base/logging_utils.source Outdated Show resolved Hide resolved
creation/web_base/logging_utils.source Outdated Show resolved Hide resolved
logserver/jwt.php Outdated Show resolved Hide resolved
logserver/jwt.php Outdated Show resolved Hide resolved
@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch 2 times, most recently from 3141a0d to fa3dbd6 Compare December 19, 2024 22:27
namrathaurs
namrathaurs previously approved these changes Dec 19, 2024
Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Changes look good and can be merged!

@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch 3 times, most recently from e8196fd to 327d3c2 Compare December 22, 2024 20:21
@mambelli mambelli requested a review from namrathaurs December 22, 2024 20:36
@mambelli
Copy link
Contributor Author

@namrathaurs Sorry for the after-review changes. Testing the RPM installed version I noticed that there were some missing parts (only in my manual deployment) and I improved also the error checking.

Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Minor comments need to be addressed.

doc/factory/custom_scripts.html Outdated Show resolved Hide resolved
doc/factory/custom_scripts.html Outdated Show resolved Hide resolved
logserver/README.md Outdated Show resolved Hide resolved
Added php and python example for a server receiving Glidein logs

Fixed JWT generation and GLIDEIN_LOG_RECIPIENTS_FACTORY param for Factory logserver, improved also code and docstrings, and added documentation for logging and log server

Refactored the logging utility to have all public functions starting with 'glog_'
Added an example custom script, logging_test.sh, using Glidein logging

Fixed mod_ssl dependency, fixed ReleaseManagerLib bugs for RPM building, and replaced the deprecated optparse with argparse

Added the glideinwms-logserver RPM and adjusted ReleaseManagerLib for it

Thanks to @namrathaurs for the review and the suggestion to remove all the example secrets

TODO: changes to url_dirs.desc require factory upgrade. Should a reconfig be sufficient?
@mambelli mambelli force-pushed the v310/add_logserver_and_fix_glidein_logging_v2 branch from 327d3c2 to 815a049 Compare December 24, 2024 17:06
@mambelli mambelli requested a review from namrathaurs December 24, 2024 17:08
Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Changes are ready for merging!

@mambelli mambelli merged commit 2a7c2c9 into glideinWMS:master Dec 26, 2024
9 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.

2 participants