Skip to content

Support adding different server locations for Microsoft face component#7532

Merged
pvizeli merged 4 commits into
home-assistant:devfrom
tsvi:ms_face_support_multi_res
May 12, 2017
Merged

Support adding different server locations for Microsoft face component#7532
pvizeli merged 4 commits into
home-assistant:devfrom
tsvi:ms_face_support_multi_res

Conversation

@tsvi
Copy link
Copy Markdown
Contributor

@tsvi tsvi commented May 10, 2017 •
edited
Loading

Description:

This fix allows to specify server_location in the configuration.yaml for microsoft_face component.
This is needed as it depends where your Azure server is located to access the API

Related issue (if applicable): fixes #7529

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2611

Example entry for configuration.yaml (if applicable):

microsoft_face:
  api_key: SECRET
  server_location: eastus2

Checklist:

  • Documentation added/updated in home-assistant.github.io
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Copy Markdown
Contributor

homeassistant commented May 10, 2017

Hi @tsvi,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@mention-bot
Copy link
Copy Markdown

mention-bot commented May 10, 2017

@tsvi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @robbiet480 and @fabaff to be potential reviewers.

pvizeli
pvizeli requested changes May 10, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please add also a unittest for that change

Comment thread homeassistant/const.py Outdated
CONF_SENDER = 'sender'
CONF_SENSOR_CLASS = 'sensor_class'
CONF_SENSORS = 'sensors'
CONF_SERVER_LOC = 'server_location'
Copy link
Copy Markdown
Member

@pvizeli pvizeli May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move that into component

Copy link
Copy Markdown
Contributor Author

@tsvi tsvi May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean directly into microsoft_face.py? I didn't know that was an option. Will implement hopefully tomorrow.

Copy link
Copy Markdown
Member

@pvizeli pvizeli May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Copy Markdown
Contributor Author

@tsvi tsvi May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread homeassistant/components/microsoft_face.py Outdated
CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.Schema({
vol.Required(CONF_API_KEY): cv.string,
vol.Optional(CONF_SERVER_LOC, default="westus"): cv.string,
Copy link
Copy Markdown
Member

@pvizeli pvizeli May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not short the constant name. So use CONF_SERVER_LOCATION. I'm not usre if CONF_API_ENDPOINT a better choise for the name. You have the choise

Copy link
Copy Markdown
Contributor Author

@tsvi tsvi May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API_ENDPOINT seems to refer the full endpoint URL.
AZURE_REGION or SERVER_REGION might be more appropriate.

Copy link
Copy Markdown
Member

@pvizeli pvizeli May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like CONF_AZURE_REGION 👍 you can choise it

Copy link
Copy Markdown
Contributor Author

@tsvi tsvi May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tsvi
Copy link
Copy Markdown
Contributor Author

tsvi commented May 10, 2017

I suppose I should update the docs as well. How do we sync between both changes?
I open a PR for next and cross reference?

@andrey-git
Copy link
Copy Markdown
Contributor

andrey-git commented May 11, 2017

@tsvi Yes, open a docs PR for next

@tsvi tsvi mentioned this pull request May 11, 2017
Merged
@tsvi tsvi changed the title Support adding different server locations for Microsoft face componenet Support adding different server locations for Microsoft face component May 11, 2017
@pvizeli pvizeli merged commit 452c3a1 into home-assistant:dev May 12, 2017
@emlove
Copy link
Copy Markdown
Contributor

emlove commented May 12, 2017

Thanks for the contribution! 🎉

@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
@tsvi tsvi deleted the ms_face_support_multi_res branch October 4, 2018 10:42
@tsvi tsvi restored the ms_face_support_multi_res branch October 4, 2018 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pvizeli pvizeli pvizeli approved these changes

Assignees

No one assigned

Labels

cla-signed component integration: microsoft_face

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Microsoft Face component should support resources in different locations

7 participants

@tsvi @homeassistant @mention-bot @andrey-git @emlove @pvizeli @balloobbot