Change sharkiqpy to sharkiq #68864

JeffResc · 2022-03-29T21:23:11Z

Proposed change

The SharkIQ integration is currently broken, see #68624.

This PR changes the underlying pypi package from sharkiqpy to sharkiq. The original maintainer of sharkiqpy seems unreachable, so I forked the repository and made the necessary changes to fix the SharkIQ vacuum integration.

Edit: If @ajmarks continues to be unavailable, I would be interested in maintaining this, as long as no one else wants to.

sharkiq Changelog

Updated const.py to reflect the new DEVICE_URL and LOGIN_URL to connect to the Shark API.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes shark stopped working 2 days ago. vacuums shown as unavailable #68624
This PR is related to issue: shark stopped working 2 days ago. vacuums shown as unavailable #68624
Link to documentation pull request: n/a

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

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

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-03-29T21:23:17Z

Hey there @ajmarks, mind taking a look at this pull request as it has been labeled with an integration (sharkiq) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

TheOneOgre · 2022-03-30T02:29:59Z

I agree that the pypi package should be updated due to @ajmarks unavailability since December of 2020. If possible, I also believe @JeffResc, your fork should have at least 1 or 2 write-access maintainers added to it on the off-chance that you disappear like @ajmarks did. (Hopefully not but things happen lol)

JeffResc · 2022-03-30T02:33:57Z

That sounds like a good idea to me as well. If anyone is interest in maintaining the project in the event of my disappearance, please let me know.

funkybunch · 2022-03-30T02:53:10Z

@JeffResc Looks like you have https://github.com/JeffResc/sharkiq/ setup to publish to pypi via Github Actions whenever a new release is cut? If thats all automated I'm happy to help maintain & review PRs.

AritroSaha10 · 2022-03-30T03:06:14Z

@JeffResc I'd also be willing to be a maintainer if any are still needed.

JeffResc · 2022-03-30T03:26:59Z

Looks like you have https://github.com/JeffResc/sharkiq/ setup to publish to pypi via Github Actions whenever a new release is cut? If thats all automated I'm happy to help maintain & review PRs.

Yep, I used that same automation on a few other repos and it works flawlessly.

@funkybunch @AritroSaha10 Added and added.

funkybunch

Since merging this PR would remove the stale sharkiq repo I'd recommend also updating lines 890 and 891 in CODEOWNERS to the maintainers of the new upstream repo: @JeffResc @funkybunch @AritroSaha10.

epenet · 2022-03-30T07:54:52Z

CODEOWNERS

The change here is unrelated - no need to add a trailing carriage return

Should be all set 👍

JeffResc · 2022-03-30T12:12:50Z

Since merging this PR would remove the stale sharkiq repo I'd recommend also updating lines 890 and 891 in CODEOWNERS to the maintainers of the new upstream repo: @JeffResc @funkybunch @AritroSaha10.

This is all set as well now

epenet · 2022-03-30T12:14:10Z

homeassistant/components/sharkiq/manifest.json

+    "codeowners": ["@ajmarks"],
+    "iot_class": "cloud_polling",
+    "loggers": ["sharkiq"]
+}


Since #68877 has been merged, please add a carriage return here, or it will fail

This should be all set now

epenet · 2022-03-30T12:36:47Z

Translation files should not be adjusted manually.
They are also ignored by prettier config file so I'm not sure why it triggered an update in your commit:

core/.prettierignore

Line 4 in cb5c7dc

homeassistant/components/*/translations/*.json

This reverts commit c2d99f2.

JeffResc · 2022-03-30T12:42:57Z

Translation files should not be adjusted manually. They are also ignored by prettier config file so I'm not sure why it triggered an update in your commit:

core/.prettierignore

Line 4 in cb5c7dc

homeassistant/components/*/translations/*.json

Oops, I did this manually as the pre-commit hooks inside my dev container don't seem to function properly, this should be fixed

epenet · 2022-03-30T12:44:52Z

I see that the new package uses very strict dependencies:
https://github.com/JeffResc/sharkiq/blob/4136e64ff3ddfa20a576909a482ecc45bcbfd044/requirements.txt#L1-2

I am a bit worried that it might cause issues in the future, when bumping those in HomeAssistant:

aiohttp==3.8.1
requests==2.27.1

JeffResc · 2022-03-30T12:47:34Z

I see that the new package uses very strict dependencies: https://github.com/JeffResc/sharkiq/blob/4136e64ff3ddfa20a576909a482ecc45bcbfd044/requirements.txt#L1-2

I am a bit worried that it might cause issues in the future, when bumping those in HomeAssistant:
aiohttp==3.8.1
requests==2.27.1

I normally pin the exact version that it's tested on just to ensure that nothing changes between the development environment, testing environment, and production environment, but I see what you mean here.

How do you feel about:

aiohttp>=3.8.1
requests>=2.27.1

epenet · 2022-03-30T13:19:58Z

How do you feel about:
aiohttp>=3.8.1
requests>=2.27.1

Sounds good

epenet · 2022-03-30T13:37:18Z

~~Separately, why is there a dependency on requests? Could it not be 100% async?~~

Sorry - it looks like you have kept both sync and async methods available. I'm generally of the view that new packages shouldn't bother with a sync version but if you are happy to continue supporting both then that's fine.

funkybunch · 2022-03-30T14:03:36Z

Separately, why is there a dependency on requests? Could it not be 100% async?

To add additional context, this upstream repo is a fork of the existing integration which is stale with the maintainer unresponsive (that repo uses synchronous methods via requests).

I think there are a few places we should refactor and I'm happy if you wanted to create an issue on the dependency's repo to move everything to asynchronous; but, the key part of this PR is to update the dependency to a repo that is actively maintained.

For now the integration is broken because the API URL changed, but so far that is the only difference between the old stale repo and this new repo.

epenet · 2022-03-30T14:07:05Z

Please rebase over latest dev to fix CI (due to #68900)

JeffResc · 2022-03-30T15:40:25Z

Please rebase over latest dev to fix CI (due to #68900)

This is all set

Separately, why is there a dependency on requests? Could it not be 100% async?

I think @funkybunch summed this up quite nicely, but there is already some discussion about this in the sharkiq repo.

engeles20 · 2022-03-30T16:24:58Z

So what do I need to do to fix? Will the update push on it's own?

epenet

Lgtm

funkybunch · 2022-03-30T16:36:36Z

So what do I need to do to fix? Will the update push on it's own?

If your home assistant instance is pulling updates from the dev channel this will update automatically once this PR is merged in. Otherwise you'll have to wait until it is rolled out in a major/minor release.

If you want to update it manually see the discussion in this issue: #68624

Just know that if you make these changes manually they will be overwritten if you update home assistant core.

JeffResc added 3 commits March 29, 2022 17:11

Change sharkiqpy to sharkiq

a3a72d7

Run gen_requirements_all

16d7613

Run hassfest

52126cb

probot-home-assistant bot added dependency integration: sharkiq small-pr PRs with less than 30 lines. labels Mar 29, 2022

homeassistant added the cla-signed label Mar 29, 2022

Fix tests

11b6a81

funkybunch suggested changes Mar 30, 2022

View reviewed changes

epenet reviewed Mar 30, 2022

View reviewed changes

JeffResc added 2 commits March 30, 2022 08:10

Fix trailing newline

d483eb9

Update sharkiq codeowners to new maintainers

d481c3f

epenet reviewed Mar 30, 2022

View reviewed changes

JeffResc added 2 commits March 30, 2022 08:24

Merge branch 'home-assistant:dev' into fix-shark

911a675

Run prettier on files

c2d99f2

JeffResc added 3 commits March 30, 2022 08:38

Revert "Run prettier on files"

fafb6d7

This reverts commit c2d99f2.

Run hassfest

6a7c09b

Fix prettier

297e107

Update sharkiq codeowners

de3bcb6

Merge branch 'home-assistant:dev' into fix-shark

41e13d7

epenet approved these changes Mar 30, 2022

View reviewed changes

epenet added the smash Indicator this PR is close to finish for merging or closing label Mar 30, 2022

balloob merged commit 7767258 into home-assistant:dev Mar 30, 2022

github-actions bot locked and limited conversation to collaborators Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change sharkiqpy to sharkiq #68864

Change sharkiqpy to sharkiq #68864

JeffResc commented Mar 29, 2022 •

edited

Loading

probot-home-assistant bot commented Mar 29, 2022

TheOneOgre commented Mar 30, 2022 •

edited

Loading

JeffResc commented Mar 30, 2022

funkybunch commented Mar 30, 2022 •

edited

Loading

AritroSaha10 commented Mar 30, 2022

JeffResc commented Mar 30, 2022 •

edited

Loading

funkybunch left a comment

epenet Mar 30, 2022

JeffResc Mar 30, 2022

JeffResc commented Mar 30, 2022

epenet Mar 30, 2022

JeffResc Mar 30, 2022

epenet commented Mar 30, 2022

JeffResc commented Mar 30, 2022

epenet commented Mar 30, 2022

JeffResc commented Mar 30, 2022

epenet commented Mar 30, 2022

epenet commented Mar 30, 2022 •

edited

Loading

funkybunch commented Mar 30, 2022 •

edited

Loading

epenet commented Mar 30, 2022 •

edited

Loading

JeffResc commented Mar 30, 2022

engeles20 commented Mar 30, 2022

epenet left a comment

funkybunch commented Mar 30, 2022 •

edited

Loading

Change sharkiqpy to sharkiq #68864

Change sharkiqpy to sharkiq #68864

Conversation

JeffResc commented Mar 29, 2022 • edited Loading

Proposed change

sharkiq Changelog

Type of change

Additional information

Checklist

probot-home-assistant bot commented Mar 29, 2022

TheOneOgre commented Mar 30, 2022 • edited Loading

JeffResc commented Mar 30, 2022

funkybunch commented Mar 30, 2022 • edited Loading

AritroSaha10 commented Mar 30, 2022

JeffResc commented Mar 30, 2022 • edited Loading

funkybunch left a comment

Choose a reason for hiding this comment

epenet Mar 30, 2022

Choose a reason for hiding this comment

JeffResc Mar 30, 2022

Choose a reason for hiding this comment

JeffResc commented Mar 30, 2022

epenet Mar 30, 2022

Choose a reason for hiding this comment

JeffResc Mar 30, 2022

Choose a reason for hiding this comment

epenet commented Mar 30, 2022

JeffResc commented Mar 30, 2022

epenet commented Mar 30, 2022

JeffResc commented Mar 30, 2022

epenet commented Mar 30, 2022

epenet commented Mar 30, 2022 • edited Loading

funkybunch commented Mar 30, 2022 • edited Loading

epenet commented Mar 30, 2022 • edited Loading

JeffResc commented Mar 30, 2022

engeles20 commented Mar 30, 2022

epenet left a comment

Choose a reason for hiding this comment

funkybunch commented Mar 30, 2022 • edited Loading

JeffResc commented Mar 29, 2022 •

edited

Loading

TheOneOgre commented Mar 30, 2022 •

edited

Loading

funkybunch commented Mar 30, 2022 •

edited

Loading

JeffResc commented Mar 30, 2022 •

edited

Loading

epenet commented Mar 30, 2022 •

edited

Loading

funkybunch commented Mar 30, 2022 •

edited

Loading

epenet commented Mar 30, 2022 •

edited

Loading

funkybunch commented Mar 30, 2022 •

edited

Loading