search

scipion-em / scipion-em-facilities

Plugin for Cryo-EM facilities to be used within Scipion framework
GNU General Public License v3.0
0 stars 1 forks source link

Adjusting parameters to the new pugin. #2

Closed dmaluenda closed 4 years ago

dmaluenda commented 4 years ago

This PR contains all the changes I made in order to accommodate the monitors to its own plugin.

I've checked the tests and they pass.

delarosatrevin commented 4 years ago

I agree with @pconesa that emfacilities is a more consistent name for the package. The overall plugin scipion-em-facilities is fine and in line with other plugin names.

rmarabini commented 4 years ago

Hi,

Secrets.py should NOT be in github. I delete it. secrets_template.py should be in the repository (I add it).

rmarabini commented 4 years ago

Hi,

The code is OK with me. I did a few modifications in the pull request

1) remove secrets.py which should be secrets 2) add a few lines of code to repoert_grafana.py that I include in the monitor after David forked it 3) fix a few typos in the comments

I think this is ready to go @dmaluenda.

dmaluenda commented 4 years ago

Ok, @rmarabini . I'm not totally confortable with the secrets. As it is, the whole plugin cannot be loaded since a failing import is found...

How do you see the following approach? putting secrets in a json, importing them in a dictionary, getting them with the 'get()' function by using default values if not found. In this way, the plugin always works and when no secrets implemented we can raise a warning with some instructions.

Another option is putting the import from secrets in a try/catch.

pconesa commented 4 years ago

Yes, we should be careful with importing things that might fail.

rmarabini commented 4 years ago

Hi,

I am not happy with secrets.py either. We certainly need something more configurable. Anyway my suggestion is, first let us finish with this pull request and them we modify the way secrets.py is uploded. Certainly, it should be a config file and we are going to need a environment variable that allow us to place it in places different from the default one.

Roberto

On Tue, May 26, 2020 at 12:30 PM David Maluenda Niubó < notifications@github.com> wrote:

Ok, @rmarabini https://github.com/rmarabini . I'm not totally confortable with the secrets. As it is, the whole plugin cannot be load since a failing import is found...

CHo do you see the following approach? putting secrets in a json, importing them in a dictionary, getting them with the 'get()' function by using default values if not found. In this way, the plugin works and when no secrets implemented we can raise a warning with some instructions.

Another option is putting the import from secrets in a try/catch.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scipion-em/scipion-em-facilities/pull/2#issuecomment-633943738, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQGTIQT5HM6TVAKKBRUVITRTOK4VANCNFSM4NJTZN7Q .