Skip to content

Conversation

@ZQyou
Copy link
Collaborator

@ZQyou ZQyou commented Feb 7, 2019

  • Support syslog handler in Logging and Performance Logging

Closes #671
Closes #699

@pep8speaks
Copy link

pep8speaks commented Feb 7, 2019

Hello @ZQyou, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated on March 01, 2019 at 12:08 Hours UTC

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@vkarak
Copy link
Contributor

vkarak commented Feb 7, 2019

@jenkins-cscs retry all

@vkarak vkarak requested review from teojgo and vkarak and removed request for vkarak February 7, 2019 16:57
@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

Merging #675 into master will decrease coverage by 0.05%.
The diff coverage is 79.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
- Coverage   91.89%   91.83%   -0.06%     
==========================================
  Files          77       77              
  Lines        9452     9494      +42     
==========================================
+ Hits         8686     8719      +33     
- Misses        766      775       +9
Impacted Files Coverage Δ
reframe/core/logging.py 83.54% <75%> (-0.69%) ⬇️
unittests/test_logging.py 97.22% <84.21%> (-1.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b21792...45f2c53. Read the comment docs.

@vkarak vkarak self-requested a review February 7, 2019 17:48
@vkarak vkarak added this to the Upcoming sprint milestone Feb 7, 2019
@vkarak
Copy link
Contributor

vkarak commented Feb 15, 2019

@ZQyou Hmm, looks like that sth has gone wrong with your branch. It seems that you have reapplied several commits already in the master. Checking the history of your branches, it seems that you have rebased over the main master and then merged again with your local master before applying your new commit. I'll see how I can fix that.

@ZQyou
Copy link
Collaborator Author

ZQyou commented Feb 18, 2019

@vkarak Apology for any inconvenience caused. Here is what I did: merge upstream master into my local master and then rebase local branch. Somehow it was asked to pull my remote branch before I push local branch then I did it.

If the issue is complicated, can I just cancel this request and submit another one?

@vkarak
Copy link
Contributor

vkarak commented Feb 18, 2019

Yes, as soon as you rebase a branch that you have already pushed, you need to do a forced update to the remote branch, because you have essentially reapplied all your commits, so the two branches have diverged according to Git. That's why it was asking you for a pull first. You can fix it by cherry-picking your commits in a clean local branch and force pushing to your remote branch.

# in your local copy
git checkout -b clean_branch
git cherry-pick COMMIT1 COMMIT2 ...
git push your_remote +clean_branch:feature/syslog_handler

The + will do the forced update on the remote branch.

* Support syslog handler in Logging and Performance Logging
* Support facility and socket type attributes
* The attribute address is now required
@vkarak vkarak force-pushed the feature/syslog_handler branch from 5e0f7fb to f98a8a9 Compare February 18, 2019 21:03
@vkarak
Copy link
Contributor

vkarak commented Feb 18, 2019

@ZQyou I fixed your PR and I forced updated into your remote branch. Can you check that I haven't missed any of your changes? In order now to update your local branch, you should not just pull, because we will end up in the same situation as before (Git will merge the remote with your local branch). You just need to do a hard reset of your local branch to the status of the remote:

# In your local repo
git checkout feature/syslog_handler
git reset --hard your_remote/feature/syslog_handler

Then you should be able to see only the two useful commits.

@ZQyou
Copy link
Collaborator Author

ZQyou commented Feb 20, 2019

@vkarak Thank you. There is no change missing. I have tested this branch on our Linux server and run a local unittest to test address='/var/run/syslog' on Mac. Both work fine.

@sekelle sekelle removed this from the ReFrame sprint 2019w07 milestone Feb 25, 2019
* Chagne the attribute `socket` to `socktype`
* Make the attribute `facility` string-based
@vkarak vkarak changed the title [feat] Add syslog handler in logging.py [feat] Add support for syslog in logging Mar 1, 2019
@vkarak
Copy link
Contributor

vkarak commented Mar 1, 2019

@ZQyou Thanks for your PR. Looks good. I will merge it as soon as the CI passes.

@vkarak
Copy link
Contributor

vkarak commented Mar 1, 2019

@jenkins-cscs retry all

Vasileios Karakasis added 2 commits March 1, 2019 12:00
@vkarak
Copy link
Contributor

vkarak commented Mar 1, 2019

@ZQyou I've also update the documentation and I have added a unit test that tests the actual logging through syslog. I've done an adaptation for OSX. Now it's ready to be merged.

@vkarak vkarak changed the title [feat] Add support for syslog in logging [feat] Add logging handler for sending records to syslog Mar 1, 2019
@vkarak vkarak merged commit b60a9fa into reframe-hpc:master Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants