search

SecConNet / mahiru

A proof-of-concept digital data exchange
Apache License 2.0
8 stars 0 forks source link

Issue 88: 2. Add connections configuration #93

Closed LourensVeen closed 3 years ago

LourensVeen commented 3 years ago

This factors out the Settings class into a separate module, and then adds a section for configuring container connections.

Please don't merge, the base branch is wrong because this is one of my famous stacked PRs.

LourensVeen commented 3 years ago

Okay, they're now SiteConfiguration and NetworkSettings. I'd like to hear what you think.

Also, are you sure you don't want to suggest that the settings should be a singleton/global object?

svenvanderburg commented 3 years ago

NetworkSettings​ sounds good!

[https://esciencecenter.nl/wp-content/themes/raadhuis/dist/assets/img/e-signature.png] Sven van der Burg (he, him, his) eScience Research Engineer/Instructor My working days are Monday, Tuesday, Wednesday, and Thursday. Netherlands eScience Center www.esciencecenter.nlhttps://www.esciencecenter.nl/

From: Lourens Veen @.> Sent: Wednesday, October 20, 2021 18:29 To: SecConNet/mahiru @.> Cc: Sven van der Burg @.>; Review requested @.> Subject: Re: [SecConNet/mahiru] Issue 88: 2. Add connections configuration (PR #93)

@LourensVeen commented on this pull request.

In mahiru/components/settings.pyhttps://github.com/SecConNet/mahiru/pull/93#discussion_r732952986:

@@ -0,0 +1,126 @@ +"""Site configuration.""" +from pathlib import Path +from typing import AnyStr, IO, List, Optional, Union + +import ruamel.yaml as yaml +import yatiml + +from mahiru.definitions.identifier import Identifier + + +class AssetConnections:

Yeah, I'm not very happy with the naming here either. Maybe NetworkSettings? That's shorter than AssetConnectionSettings but still covers the contents.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHubhttps://github.com/SecConNet/mahiru/pull/93#discussion_r732952986, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACL4BJ3B3GFNZSOK637KYNLUH3VALANCNFSM5GJH2SOA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

svenvanderburg commented 3 years ago

Okay, they're now SiteConfiguration and NetworkSettings. I'd like to hear what you think. Also, are you sure you don't want to suggest that the settings should be a singleton/global object? I thought I could get away with responding to Github emails only, but that clearly doesn't work!

I really like the new names! And about singletons: In this case it seems like the SiteConfiguration and NetworkSettings are only used in the Site object (I only had a quick look though), which makes sense because you need those settings to setup the site correctly. So I would argue against a singleton. If it is (or becomes) more like a global configuration then indeed it would make sense to create a singleton.

LourensVeen commented 3 years ago

Ah, merged it but forgot to adjust the reference branch here, so GitHub didn't detect that. Closing.