Skip to content

Conversation

@fmherschel
Copy link
Member

No description provided.

@fmherschel fmherschel requested a review from arbulu89 December 3, 2020 09:27
@fmherschel
Copy link
Member Author

@arbulu89 As discussed in the previous PR I have splitted now the fix for the bad return code from the new feature (pre/post service) coming with the next PR

Copy link
Collaborator

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Hi @fmherschel
We should put that second big comment in the main docstring.
It makes a visual impresion that the code is splitted.

Besides that, I still see the need to cover this new piece of code by unit test, as the new if/else is not.
Let me know if you need any help with the tests

@fmherschel
Copy link
Member Author

fmherschel commented Dec 3, 2020

** removing my comments here and will come back with an updated one **
I have overseen that this is github not an internal channel, sorry!

@arbulu89
Copy link
Collaborator

arbulu89 commented Dec 3, 2020

@fmherschel I'm sorry if I gave the impression that I'm blocking your efforts. It was not my intention.
As an "external" contributor who doesn't have that much knowledge about the project, my unique intention was to redirect your changes to follow python best practices.

This just meant:

  1. To unify the docstring in method docstring. Something like:
def monitor(self):
        '''
        Is the sapstartsrv server process running?

        For regular monitors always return success, because recover of sapstartsrv is already handeled by SAPInstance
        This might be changed in a next-generation edition
        '''
  1. To create just one additional UT to cover the 100% of the code.

As they were just small changes, I thought we should add them.

If we have that many urgency, we can just merge this PR, opening a follow up github ticket commenting the needed improvements, so we can work on them later. I could do that myself.
Believe me, that everything I try to do is to have the best quality code we can have. Nothing else

@fmherschel
Copy link
Member Author

@arbulu89 Thanks for you understanding the pressure. We should have a short call. Lets communcate the other stuff via RC. I was running into the trap mixing the communication channels. Sorry for that.

@fmherschel fmherschel merged commit 81156db into SUSE:master Dec 3, 2020
@arbulu89
Copy link
Collaborator

arbulu89 commented Dec 3, 2020

Follow up ticket: #11

@arbulu89 arbulu89 linked an issue Dec 3, 2020 that may be closed by this pull request
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.

SAPStartSrv returns OCF_ERR_GENERIC (rc=1)

3 participants