Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub integration revamp #38458

Closed
wants to merge 43 commits into from
Closed

Conversation

timmo001
Copy link
Contributor

@timmo001 timmo001 commented Aug 1, 2020

Breaking change

YAML config has been removed. This is replaced with the configuration in the user interface.
The sensor has also been split into individual sensors for each type with a subset enabled to start. You can enable more via the integration's options.

Proposed change

A rewrite of the GitHub integration with a config flow, coordinator, split sensors and a new package without I/O in the event loop.

image

image
Clones and Views are removed from the options flow for repos without push access. Only latest commit is selected for initial setup. (to reduce API hits)

image

Type of change

  • New feature (which adds functionality to an existing integration)

Additional information

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:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • 🥈 Silver

@timmo001

This comment has been minimized.

@frenck
Copy link
Member

frenck commented Aug 3, 2020

Really cool, was thinking about this myself the other day, mainly to have webhook support or things like services to create issues (out of scope for now 😉)

Nevertheless, GitHub support OAuth2, why not leverage that?

@timmo001
Copy link
Contributor Author

timmo001 commented Aug 3, 2020

Really cool, was thinking about this myself the other day, mainly to have webhook support or things like services to create issues (out of scope for now 😉)

Well that's in the package, so yeah, future enhancement 😉

Was also thinking about adding actions/ci if that's accessable, but again, keep this simple, add later 😀

Nevertheless, GitHub support OAuth2, why not leverage that?

I'll take a look. Main reason was simplicity 😄

Edit: I find that tokens are much simpler and don't require multiple IDs and a new app creating. Feel free to add of course but I prefer the current method for this pr

@timmo001 timmo001 changed the title WIP: GitHub integration revamp GitHub integration revamp Aug 3, 2020
@timmo001 timmo001 marked this pull request as ready for review August 4, 2020 16:43
@stale
Copy link

stale bot commented Sep 5, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added stale and removed stale labels Sep 5, 2020
@timmo001 timmo001 force-pushed the github-revamp branch 4 times, most recently from e8d919c to dcf97d0 Compare September 5, 2020 23:34
Copy link
Contributor

@davet2001 davet2001 left a comment

Choose a reason for hiding this comment

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

I have made a start on this, looking a the non .py files first. Minor comments.

Big PR but (to encourage others) approx 360lines are a json test fixture.

Needs breaking change tag adding.

Comment on lines +1 to +48
{
"title": "GitHub",
"config": {
"flow_title": "GitHub: {repository}",
"step": {
"reauth": {
"title": "GitHub: Reauthentication",
"description": "Please re-enter your [access token](https://github.com/settings/tokens).",
"data": {
"access_token": "Access Token"
}
},
"user": {
"description": "Please enter your [access token](https://github.com/settings/tokens) and repository (`org/project`).",
"data": {
"access_token": "Access Token",
"repository": "Repository"
}
}
},
"error": {
"cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
"cannot_find_repo": "Cannot find repository",
"invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
"unknown": "[%key:common::config_flow::error::unknown%]"
},
"abort": {
"already_configured": "[%key:common::config_flow::abort::already_configured_account%]",
"connection_error": "There was an error connecting to GitHub.",
"reauth_successful": "[%key:common::config_flow::data::access_token%] updated successfully"
}
},
"options": {
"step": {
"user": {
"title": "GitHub Options",
"description": "To help reduce the chances of hitting API limits, some items have been disabled by default. You can enable and disable these items here.",
"data": {
"clones": "Clones",
"issues_and_prs": "Issues and Pull Requests",
"latest_commit": "Latest Commit",
"latest_release": "Latest Release",
"views": "Views"
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the en translation is not needed in the commit.

Comment on lines +9 to +12
"ssdp": [],
"zeroconf": [],
"homekit": {},
"dependencies": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be ommitted if blank?

@ludeeus
Copy link
Member

ludeeus commented Jan 13, 2021

We should not hard break it to add config flow, there should be a grace period of 2 releases where the configuration is imported.
This also removes the URL config option, which makes it no longer working with enterprise instances is there a reason for that?

@github-actions
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 12, 2021
@timmo001
Copy link
Contributor Author

I'll revisit this at some point. I've seen a few package updates also

@github-actions github-actions bot removed the stale label Feb 15, 2021
@timmo001
Copy link
Contributor Author

timmo001 commented Mar 13, 2021

Think I'll close this and let someone else take it or come back to it at some point since its became pretty old and stale at this point 😸

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.

5 participants