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

[v6r20] Bug fixes for pilot 3 files synchronization #3837

Merged
merged 6 commits into from
Sep 21, 2018

Conversation

fstagni
Copy link
Contributor

@fstagni fstagni commented Sep 18, 2018

Plus other minor fixes

BEGINRELEASENOTES

*Configuration
FIX: Corrected configuration location for Pilot 3 files synchronization

ENDRELEASENOTES

@coveralls
Copy link

coveralls commented Sep 18, 2018

Coverage Status

Coverage decreased (-0.004%) to 22.258% when pulling d7d9cf2 on fstagni:v6r20-fixes37 into 456ff2f on DIRACGrid:rel-v6r20.

return backupsList

def __getPreviousCFG( self, oRemoteConfData ):
remoteExpectedVersion = oRemoteConfData.getVersion()
Copy link
Contributor

@atsareg atsareg Sep 21, 2018

Choose a reason for hiding this comment

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

This changes the semantics. Now the version is evaluated once at the moment the function definition is processed. Previously, the version was reevaluated before each function call. And I think in this case this really matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understand: why would it matter? I can restore it, no problem, but I would like to understand what's the reason, as remoteExpectedVersion is not used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right, I thought the subsequent call was using this variable anticipating the developer intentions. And I think this will be indeed correct to get the value into the variable and pass it to the subsequent self.__getCfgBackups(... call.

rpcClient = self.__getRPCClient()
return rpcClient.setAssigneeGroup( groupName, userList )

def getUsersInAssigneeGroup( self, groupName ):
Copy link
Contributor

Choose a reason for hiding this comment

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

You drop these methods because there are implicitly accessible through the generic client anyway ? If so, then we rather move in a different direction to specify explicitly all methods in a specific client in order to be able to specify individual timeout, for example, and also to make those methods visible at the iPython prompt.

Copy link
Contributor Author

@fstagni fstagni Sep 21, 2018

Choose a reason for hiding this comment

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

Yes I removed them because I am using Client as base class.
To have calls visible through iPython we should improve the Client class instead: some time ago it was tried already https://github.com/DIRACGrid/DIRAC/blob/integration/Core/Base/Client.py#L34

@fstagni
Copy link
Contributor Author

fstagni commented Sep 21, 2018

For this one: I need to update the wiki for the change in the CS locations. Let me know if you agree with what I have done (module the comment in the other module)

@atsareg
Copy link
Contributor

atsareg commented Sep 21, 2018

Sorry, I have not understand the question in the previous comment "(module the comment in the other module)" ?

@fstagni
Copy link
Contributor Author

fstagni commented Sep 21, 2018

Sorry, I was not clear: the changes in PilotCS2JSONSynchronizer need to be added in the wiki, let me know if you agree with them.

@atsareg
Copy link
Contributor

atsareg commented Sep 21, 2018

Yes, I agree, please, add to the Wiki

@atsareg atsareg merged commit 2551367 into DIRACGrid:rel-v6r20 Sep 21, 2018

class ServiceInterface(threading.Thread):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need to format this file ? Louis modified it in his PR, and this is for sure going to be a merging conflict for Andrei.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been merged already...

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.

4 participants