Add a dogshell option to change Datadog site to call API#691
Conversation
|
@DataDog/integrations-tools-and-libraries can anyone review please? |
therve
added
the
changelog/Added
therve
left a comment
therve
left a comment
There was a problem hiding this comment.
Please fix the lint errors as well.
datadog/dogshell/__init__.py
Outdated
| "--site", | ||
| help="Datadog site to send data, us (datadoghq.com), eu (datadoghq.eu) or us3 (us3.datadoghq.com), default: us", | ||
| dest="site", | ||
| default=os.environ.get("DD_SITE", os.environ.get("DATADOG_HOST")), |
There was a problem hiding this comment.
Site and host have different semantic, I don't think we should use the same env var for this.
There was a problem hiding this comment.
I see, I will change it to api_host
| help="Datadog site to send data, us (datadoghq.com), eu (datadoghq.eu) or us3 (us3.datadoghq.com), default: us", | ||
| dest="site", | ||
| default=os.environ.get("DD_SITE", os.environ.get("DATADOG_HOST")), | ||
| choices=["datadoghq.com", "us", "datadoghq.eu", "eu", "us3.datadoghq.com", "us3"], |
There was a problem hiding this comment.
Are you sure we want support us and eu ? That could be confusing with other datacenters.
There was a problem hiding this comment.
I thought we can be aligned with https://github.com/DataDog/datadogpy/blob/master/datadog/dogshell/wrap.py#L289
What do you think? Should we remove us, eu, us3?
There was a problem hiding this comment.
Thanks I will leave it.
| elif site in ("datadoghq.eu", "eu"): | ||
| self["api_host"] = "https://datadoghq.eu" | ||
| elif site in ("us3.datadoghq.com", "us3"): | ||
| self["api_host"] = "https://us3.datadoghq.com" |
There was a problem hiding this comment.
We should probably raise an error if the value doesn't match.
There was a problem hiding this comment.
Error is thrown by choices=["datadoghq.com", "us", "datadoghq.eu", "eu", "us3.datadoghq.com", "us3"]
Error message will be like
__init__.py: error: argument --api_host: invalid choice: 'us4' (choose from 'datadoghq.com', 'us', 'datadoghq.eu', 'eu', 'us3.datadoghq.com', 'us3')
Is it cool?
|
@therve Updated, can you review please? |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
therve
changed the title
|
I know this has already been merged but just a tiny comment. The new option --api_host uses an under score where both other api options just uses a hyphen. |
3 participants
What does this PR do?
Issue to fix
#652
Description of the Change
Adding
--siteoption to change the site to call the APIVerification Process
Tested with my sandbox account in EU and US3 site.