Use objects for CloudEndpoints and CloudSuffixes instead of dicts #1145
Conversation
There was a problem hiding this comment.
Overall I think it cleans up the logic nicely. We might add some unit tests to validate what happens if you register a cloud with a "None" endpoint to ensure things fail gracefully, and also to test the code path when creating a configuration from file (a la Stack) instead of the common clouds.
| AZURE_DATALAKE_ANALYTICS_CATALOG_AND_JOB_ENDPOINT = 'azure_datalake_analytics_catalog_and_job_endpoint' #pylint: disable=line-too-long | ||
| class CloudEndpoints(object): | ||
| def __init__(self, | ||
| management=None, |
There was a problem hiding this comment.
Instead of None, should these default to public Azure? For example if I had a hybrid cloud (Stack) setup where I had one service running on my Stack instance, but wanted the rest to just come from public?
There was a problem hiding this comment.
I don't think it makes sense for them to default to public Azure...
In that scenario you would pass in all the endpoints you need. If some of them happen to be the same as public Azure then so be it.
Also, when I register a cloud, I would not expect for the values I didn't specify to be set to point to public Azure.
There was a problem hiding this comment.
Makes sense. Since all of our common cloud objects have everything filled out, I think we should test the scenario where one of these is None to ensure they fail gracefully.
derekbekoe
force-pushed
the
cloud-endpoint-suffix-obj
branch
from
62af195 to
52cba5e
Compare
| if cloud.suffixes[suffix]: | ||
| config.set(cloud.name, 'suffix_{}'.format(suffix), cloud.suffixes[suffix]) | ||
| for k, v in cloud.endpoints.__dict__.items(): | ||
| if v: |
There was a problem hiding this comment.
will that ever be a case that we could have an entry w/o a value?
There was a problem hiding this comment.
Yes.
I don't want to save the values that weren't set by the user to the cloud config file.
e.g. If my cloud doesn't support Data Lake, there's no need to set the Data Lake endpoint.
| @@ -108,3 +108,7 @@ def to_camelcase(s): | |||
| if not callable(v) and not k.startswith('_')]) | |||
| else: | |||
| return obj | |||
|
|
|||
| def to_snake_case(s): | |||
There was a problem hiding this comment.
i suggest put in couple of tests on this, particular if the name is already separated by '_', are we adding a redundant one? Of course, if that will never happen, then you can just add a simple comment.
|
LGTM |
|
I would still like 2 additional tests:
|
|
@tjprescott Added more tests. Please take a look. |
|
For test (1), I verify the loading of the endpoints with a call to profile which attempts to load endpoints.management. There is a unit test to verify that clouds can be added, listed and then deleted appropriately. This verifies (2) in your comment. |
This is an internal change and doesn't affect the 'az cloud' or 'az context' commands
- Raise exception if attempting to use endpoint/suffix that is not set
derekbekoe
force-pushed
the
cloud-endpoint-suffix-obj
branch
from
6522ea2 to
b659bc2
Compare
Making this change based off feedback from the previous PR #1098 (comment)
(this is an internal change and doesn't affect the 'az cloud' or 'az context' commands)