search

project8 / dripline-python

python implementation of project8/dripline
Other
2 stars 0 forks source link

import AMQP broker from .project8_authentication.json #16

Open guiguem opened 7 years ago

guiguem commented 7 years ago

Because it is convenient for dripline-web (and dripline-labview actually), the amp broker should be given in the .project8_authentication file instead of the config file. The broker should be read in service.py, right before connecting (in connect method). The import of the broker from the config file should then be depreciated.

guiguem commented 7 years ago

actually related to #15

laroque commented 7 years ago

Per discussion on the last analysis call, the challenge is to specify exactly what behavior we expect (ie how is the existing featured deprecated?). My understanding of that discussion was that we decided on 1 below, not 2, at least for now.

  1. If we do it by convention only, by removing those lines from the config files (but it remains a valid parameter if it is there), then for service.py the solution is to modify self.__get_credentials so that it also updates the value of self._broker if ((self._broker is None) and ()). We would also need to add 'localhost' as the default here (and set the dragonfly argument default to None, or perhaps None if no flag is given and 'localhost' if the flag is given with no argument?). Here the deprecation itself happens only in the hardware repo.

  2. If we want broker values specified in the config file to be ignored or raise errors then that is a change to how the configuration files are processed, we'd have to add a validation step when the files are parsed that either checks for disallowed configurations and removes them or raises the exception, this would have to be done in dragonfly (and wherever config files are processed in golang and c++) because by the time the config reaches service, there is no distinction between it coming from the shell or the file. This solution requires a major version bump across all software because they all share the authentication file and would all need to have this requirement.

guiguem commented 7 years ago

We could go for convention only now and implement it deeper in dragonfly later, since this will require to remove the broker from the config file to avoid errors and more testing than what we need. Or we could decide that this change is something to be done in the future... Does it make sense?