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

Fortios device tracker updates #92331

Merged
merged 57 commits into from Jun 15, 2023
Merged

Conversation

kimfrellsen
Copy link
Contributor

@kimfrellsen kimfrellsen commented May 1, 2023

Proposed change

Updated Fortios device tracker with bugfixes which caused the device tracker to no respond any values due to a change in the API format returned by FortiOS.
Performance uptimization.

Thanks to @julian0703 for helping with finding the bug, suggestions and testing.

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)
  • Deprecation (breaking change to happen in the future)
  • 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 #
  • This PR is related to issue:
  • Link to documentation pull request:

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
  • I have followed the perfect PR recommendations
  • 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.
  • 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.

To help with the load of incoming pull requests:

kimfrellsen and others added 30 commits June 8, 2021 09:54
Added support for FortiOS 7.0 and retaining FortiOS 6.4 support.
Since an API was deprecated in FortiOS 7.0 and replace by a new API the integration now also support FortiOS 7.0.
It is planned to deprecate the support for FortiOS 6.4 in a year
indentation fix
run flake8 fixes
black fixing line breaks
removed comment that pylint does not like :-~
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
to resolve double guard for supported versions.
Deprecated old api.
cleaned up code.
better checking with try-catch
removed unnecessary error output.
lint compliance.
updated to use awesomeversion component.
bug fix. Check on json keys before accessing them.
performance optimise, by filtering on the parameters to fetch from FortiGate.
performance optimisation, bug fix and update to work with HASS 2023.4
@kimfrellsen kimfrellsen marked this pull request as ready for review June 12, 2023 10:46
Copy link
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.

I suggest reverting all the refactoring and just make a PR that fixes the open issue. The refactoring is making the code harder to read and generally less succinct.

homeassistant/components/fortios/device_tracker.py Outdated Show resolved Hide resolved
homeassistant/components/fortios/device_tracker.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft June 12, 2023 11:11
reverted all refactoring and only fixed the open issue.
@kimfrellsen kimfrellsen marked this pull request as ready for review June 15, 2023 12:15
@kimfrellsen
Copy link
Contributor Author

Thanks for the good improvements. all resolved.

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Copy link
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.

One comment remaining.

#92331 (comment)

@home-assistant home-assistant bot marked this pull request as draft June 15, 2023 13:26
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Copy link
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.

Thanks!

@MartinHjelmare MartinHjelmare marked this pull request as ready for review June 15, 2023 13:33
@MartinHjelmare MartinHjelmare added this to the 2023.6.3 milestone Jun 15, 2023
@MartinHjelmare MartinHjelmare merged commit 562f0d3 into home-assistant:dev Jun 15, 2023
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2023
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.

None yet

4 participants