Skip to content

Migrate CONF_WEBHOOK_ID to homeassistant.const#17460

Merged
balloob merged 5 commits intohome-assistant:devfrom
rohankapoorcom:conf-webhook-id
Oct 15, 2018
Merged

Migrate CONF_WEBHOOK_ID to homeassistant.const#17460
balloob merged 5 commits intohome-assistant:devfrom
rohankapoorcom:conf-webhook-id

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Oct 14, 2018

Description:

CONF_WEBHOOK_ID = 'webhook_id' is currently defined in the webhook automation as well as the ifttt component. As part of #15376, I was going to add it to the mailgun component as well (and it will likely be needed in other components as well as they migrate to use the new webhooks). I think it makes more sense to be be a part of the homeassistant.const package since it is shared.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

Copy link
Copy Markdown
Member

@fabaff fabaff 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 to me 🐦

@rohankapoorcom
Copy link
Copy Markdown
Member Author

@fabaff - I noticed a couple of instances in ifttt where webhook_id is used directly as a string still (example). Okay to swap it over to the constant in this pull request or would a new one be preferred for that?

@fabaff
Copy link
Copy Markdown
Member

fabaff commented Oct 14, 2018

You can do it in this PR, would be faster than waiting for two round of CI to pass.

@rohankapoorcom
Copy link
Copy Markdown
Member Author

Great, should be ready to go then!

result = {
'platform': 'webhook',
'webhook_id': webhook_id,
CONF_WEBHOOK_ID: webhook_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This better keep it as string literal, since it is not part of configuration.

@awarecan
Copy link
Copy Markdown
Contributor

I don't think we should change the webhook_id used out of context of configuration to CONF_WEBHOOK_ID e.g., those usage in your last two commits

@rohankapoorcom
Copy link
Copy Markdown
Member Author

rohankapoorcom commented Oct 15, 2018

I don't think we should change the webhook_id used out of context of configuration to CONF_WEBHOOK_ID e.g., those usage in your last two commits

That was one of the things I was also concerned about. Is it acceptable to create a new constant under the ID section and then use that for CONF_WEBHOOK_ID as well as the places that are not actually configuration?

Something like this:

WEBHOOK_ID = 'webhook_id`'
CONF_WEBHOOK_ID = WEBHOOK_ID

@balloob
Copy link
Copy Markdown
Member

balloob commented Oct 15, 2018

No. Just leave the automation as-is. Let's not start aliasing constants.

@ghost ghost assigned balloob Oct 15, 2018
@balloob balloob merged commit bd450ee into home-assistant:dev Oct 15, 2018
@ghost ghost removed the in progress label Oct 15, 2018
@rohankapoorcom rohankapoorcom deleted the conf-webhook-id branch October 15, 2018 13:36
@balloob balloob mentioned this pull request Oct 26, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
@ghost ghost added the integration: webhook label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants