SUSE / salt-shaptools

Salt execution module and state to manage SAP Applications (HANA only at the moment) and SUSE Linux Enterprise High Availability components
Apache License 2.0
14 stars 11 forks source link

Add drbd module and state. #17

Closed nick-wang closed 5 years ago

nick-wang commented 5 years ago

@arbulu89 I have fixed some of the issue in codeclimate. However, still some of the issues report of the latest code codeclimate. I don't think fix them make the code easier to understand/maintain, since code in upstream share the same style. What 's your opinion?

arbulu89 commented 5 years ago

@arbulu89 I have fixed some of the issue in codeclimate. However, still some of the issues report of the latest code codeclimate. I don't think fix them make the code easier to understand/maintain, since code in upstream share the same style. What 's your opinion?

Hi @nick-wang, Don't worry about them. Codeclimate complains about everything, and most salt related code always will have some issues

diegoakechi commented 5 years ago

Ok, it was too good to be true... @nick-wang can you rebase your branch.. we did the changes to rename the repo and it broke the changes you did on the RPM package.

nick-wang commented 5 years ago

Ok, it was too good to be true... @nick-wang can you rebase your branch.. we did the changes to rename the repo and it broke the changes you did on the RPM package.

Sure, will do it tomorrow. Thanks a lot for the change. I know fix all the dependencies in all repos/pkgs is not as easy as a simple sed

nick-wang commented 5 years ago

Ok, it was too good to be true... @nick-wang can you rebase your branch.. we did the changes to rename the repo and it broke the changes you did on the RPM package.

Finished rebase on the latest change.

diegoakechi commented 5 years ago

@nick-wang Can you include the missing coverage for DRDB? If is it too complex, we can accept this way, but if not, I think it worth to do the improvement before merge.

nick-wang commented 5 years ago

@nick-wang Can you include the missing coverage for DRDB? If is it too complex, we can accept this way, but if not, I think it worth to do the improvement before merge.

Will try~ I was trying to find where exactly 3% it point to, but i didn't find a report with details:( Do you have any idea on where i can see it?

nick-wang commented 5 years ago

Enhance test coverage to 100%

nick-wang commented 5 years ago

@arbulu89 Appreciate your valuable idea. This is a summary of my fixes based on your comments to ease the review. I fixed most of the comments, except:

  1. Remove the global keyword by salt built-in __salt__['drbd.resource']: This was failed in my previous test due to different behavior, i will dig more to see the possibility of replacement.
  2. Remain using subcases instead of different functions. Refer to comment above for details. (I need to fix the incompatible of deepcopy in python2 later)
  3. For the two functions that support json format only. So far, i only mentioned it in function's __doc__, do you think it is better to add a warning/error to log when calling without json?

Thanks a lot for the review. It is really helpful:)

nick-wang commented 5 years ago
  • Remove the global keyword by salt built-in __salt__['drbd.resource']: This was failed in my previous test due to different behavior, i will dig more to see the possibility of replacement. Fixed in 7a7ebc2c70efeac8cc15a098f861ebe499d2758d via __context__
  • Remain using subcases instead of different functions. Refer to comment above for details. (I need to fix the incompatible of deepcopy in python2 later) Fixed in 81287f072540ab7d713af09d0a88363e42d94079 by creating multiple instances.

@arbulu89 I think i fixed all the spots that you comment, please help to review. Thanks in advanced.

nick-wang commented 5 years ago

@arbulu89 @diegoakechi Thanks a lot for the review and your opinions. They are valuable and make the code better.

I will create a new pr later for removing the json condition, since it is the only support method in the near plan.

nick-wang commented 5 years ago

Actually, I didn't know this __context__ usage, but maybe it fits better than using __salt__

Me too. I just learned and referenced the context usage today, some modules use it to remove global variable.