Improve debian reposync logging (bsc#1227859) #9705
Conversation
meaksh
force-pushed
the
master-bsc1227859
branch
from
efa3ad8 to
52f4ad7
Compare
| # Apparently we need to call initCFG and have CFG available | ||
| # to prevent some errors accessing CFG later on during package | ||
| # import. | ||
| initCFG("server.satellite") |
There was a problem hiding this comment.
Can we please fix this as well? I'm okay with doing it in another PR, as we already have the patch we shouldn't leave it lying around until it does not apply anymore.
There was a problem hiding this comment.
I'd vote for doing a separated PR.
In our initial attempt, we noticed that other python files that are part of the reposync execution stack were relying on CFG being preinitializated to server.satellite, and that causes some issues in certain corner cases. Instead of going forwad with the refactor, in the context of the bug fix, we just isolated the actual logging improvement from the refactor to use context manager (that was causing some issues). I agree we can just fix them, but we didn't by then.
I would say we can prepare another PR with the refactor, plus any extra fixes required to use context managers, just after this one.
There was a problem hiding this comment.
You're adding a lot of pylint disable comments. What's the plan for those?
There was a problem hiding this comment.
Apparently this spacewalk-repo-sync file is been excluded from our Python linting checks, as not ending with .py extension, so it never got a linting execution. I'm adding a commit to this PR to enable linting on more files.
I just did it and got a couple of issues (as baseline was never set for this file). I agree we should tackle those at some point, as per other files, but I just didn't include it as part of this PR.
There was a problem hiding this comment.
Changing our lint.sh is only part of the story, the GH Action uses a .py filter. Having these executable scripts commit to the repository does not follow current best practices (where entry points are defined in project metadata and generated during package installation), but that's a different topic.
I think we should think about the filtering we do in GH Actions. The whitelist approach is getting a bit cumbersome lately where we miss out on linting (but still get a green checkmark!).
meaksh
force-pushed
the
master-bsc1227859
branch
2 times, most recently
from
76bfcdc to
1b2cb83
Compare
python/spacewalk/common/repo.py
Outdated
| logger.error( | ||
| "%s: %s. Raising GeneralRepoException.", | ||
| DpkgRepo.GPG_VERIFICATION_FAILED, | ||
| release_file, |
There was a problem hiding this comment.
There are other instances of this as well. I'll fix it and force push with a clean history
agraul
force-pushed
the
master-bsc1227859
branch
3 times, most recently
from
4edf47d to
61defe6
Compare
Since all loggers in the logging library inherit the config from their parents up to the root logger, we can use the root logger similarly to rhnLog's LOG object. The root logger singleton is changed to direct the log messages to the same log file with a similar log level as LOG.
agraul
force-pushed
the
master-bsc1227859
branch
from
61defe6 to
98aa518
Compare
python/linting/lint.sh
Outdated
| function main() { | ||
| ensure_latest_container_image | ||
| if [[ "${CHECK_ALL_FILES}" == "true" ]]; then | ||
| files="$(get_all_py_files)" | ||
| files="$(get_all_py_files)$(get_all_files_with_python_shebang)" |
There was a problem hiding this comment.
Should we add a space here just to make sure the last get_all_py_files and the first get_all_files_with_python_shebang don't merge?
Also, I suspect that both methods might return the same files, so one file might be listed twice. This didn't cause issues (although a bit annoying perhaps in logs), right?
| files="$(get_all_py_files)$(get_all_files_with_python_shebang)" | |
| files="$(get_all_py_files) $(get_all_files_with_python_shebang)" |
There was a problem hiding this comment.
I guess it makes sense to split these, thanks!
I'm sure if we want to include this commit at all, on PR we're not checking those files and I'd like to keep the number of commits that reformat down (so that we can add them to .git-blame-ignore-revs easily). It's something we should revisit shortly either way.
|
@m-czernek I think I addressed all your open points now. Please take another look when you have time. BTW, the checkstyle now failed for the test (I updated it -> it get's checked) because black is reformatting parts I didn't touch. I think I saw this before when we updated the black version in the container, I just hope this doesn't happen all the time |
There was a problem hiding this comment.
Perfect, +1 from my side :) (though we need to squash fixups, right?)
Regarding the checkstyle, I've seen that before, but IMO this is unlikely to have been caused by a new Black version - trailing comma was a formatting style even when I first formatted our Python parts. I can't explain how this has been missed, honestly.
What does this PR change?
This PR refactors and enhances logging around debian reposync executions.
GUI diff
No difference.
Documentation
No documentation needed: only internal and user invisible changes
DONE
Test coverage
No tests: no tests around reposync logging
DONE
Links
Issue(s): https://github.com/SUSE/spacewalk/issues/24816
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test(see below)Re-run a test
If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:
Before you merge
Check How to branch and merge properly!