Skip to content

Rebrand Cisco Spark notify to be Cisco Webex Teams#21938

Merged
robbiet480 merged 16 commits intohome-assistant:devfrom
fbradyirl:sparkRebrand
Apr 3, 2019
Merged

Rebrand Cisco Spark notify to be Cisco Webex Teams#21938
robbiet480 merged 16 commits intohome-assistant:devfrom
fbradyirl:sparkRebrand

Conversation

@fbradyirl
Copy link
Copy Markdown
Contributor

@fbradyirl fbradyirl commented Mar 11, 2019

Description:

Since ciscosparkapi pypi will be unsupported towards the end of 2019, it is a good idea to move to the replacement webexteamssdk pypi module.

Pull request in home-assistant.io with documentation: home-assistant/home-assistant.io#8919

Example entry for configuration.yaml:

notify:
  - name: NOTIFIER_NAME
    platform: cisco_webex_teams
    token: YOUR_BOT_TOKEN
    room_id: CISCO_WEBEX_TEAMS_ROOMID

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@fbradyirl
Copy link
Copy Markdown
Contributor Author

cc @shenning00

@andrewsayre
Copy link
Copy Markdown
Member

The only breaking change to the config is the platform name. What do we think about retaining the old platform but updating it to print a deprecation warning and then import the new platform's logic? Not sure how feasible that is, but, it would give folks time to adjust their config before things break. @balloob thoughts?

@fbradyirl
Copy link
Copy Markdown
Contributor Author

The only breaking change to the config is the platform name. What do we think about retaining the old platform but updating it to print a deprecation warning and then import the new platform's logic? Not sure how feasible that is, but, it would give folks time to adjust their config before things break. @balloob thoughts?

Personally, I would be in favour of a clean break. The user would see in the logs that the platform doesn't exist, and go from there. However, I am not against a type of platform deprecation if such a method exists, or has happened in the past?

@awarecan
Copy link
Copy Markdown
Contributor

You can leave a mock spark platform, only purpose of it is to print out warning message with how to migrate instruction.

@fbradyirl
Copy link
Copy Markdown
Contributor Author

would there be a way to tell the user via Notification Center? Or must we rely on them checking the logs?

@awarecan
Copy link
Copy Markdown
Contributor

The error message can be trigger by schema validation, that is in very early stage of start up, user can also validate their current config by our command line script, and even before they upgraded by hassio check config add-on

So, a configuration validation error is better IMO.

@fbradyirl fbradyirl closed this Mar 13, 2019
@fbradyirl fbradyirl reopened this Mar 13, 2019
@ghost ghost added in progress and removed in progress labels Mar 13, 2019
@fbradyirl
Copy link
Copy Markdown
Contributor Author

Are folks happy with this PR or do you require changes?

Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

Please rebase on latest dev and solve the merge conflict.

@ghost ghost assigned fbradyirl Apr 1, 2019
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare 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! Add the component package to .coveragerc, and see my comment below.

Can be merged when build passes and those things are fixed.

Comment thread homeassistant/components/cisco_webex_teams/notify.py Outdated
@MartinHjelmare
Copy link
Copy Markdown
Member

We should remove the breaking change label, right?

@robbiet480 robbiet480 merged commit a7d49e4 into home-assistant:dev Apr 3, 2019
@ghost ghost removed the in progress label Apr 3, 2019
@awarecan
Copy link
Copy Markdown
Contributor

awarecan commented Apr 3, 2019

Still breaking change?

@robbiet480
Copy link
Copy Markdown
Contributor

Nope, @MartinHjelmare already said we could remove the label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants