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

Replace redfish discover method #69

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

dashmage
Copy link
Contributor

@dashmage dashmage commented Oct 2, 2023

redfish.discover_ssdp method is replaced with a simple login call to the redfish service. If the RetriesExhaustedError exception is raised, we can infer that the redfish service isn't present on the system.

This is done because the redfish.discover_ssdp method finds all the redfish services on the same network which is not what we want.

Fixes #61.

This is done because redfish.discover_ssdp() discovers all available
redfish services on the same network.

This change checks if redfish is available by trying to login and check
for the type of exception being raised. If RetriesExhaustedError is
raised, it means that there is no redfish service.

Resolves canonical#61.
The "on_remove_event" test was failing because after the relation
between the ubuntu and hw-observer charm was removed, the ubuntu
app would be active/idle and settle down before the cleanup activities
were completed for hw-observer.
@dashmage dashmage requested review from a team, Pjack, aieri, esunar, agileshaw, jneo8 and rgildein October 2, 2023 05:06
@dashmage
Copy link
Contributor Author

dashmage commented Oct 2, 2023

Unit tests failing on GH CI for test coverage reasons but doesn't fail on my local 😕

image

@jneo8
Copy link
Contributor

jneo8 commented Oct 2, 2023

Unit tests failing on GH CI for test coverage reasons but doesn't fail on my local 😕

image

The failing is because of the coverage missing.

Copy link
Contributor

@jneo8 jneo8 left a comment

Choose a reason for hiding this comment

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

LGTM but please provide more unit tests to make unit test coverage be 100%.

Copy link

@Pjack Pjack left a comment

Choose a reason for hiding this comment

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

There is no unit test of redfish_available, any reason to skip it?

src/hw_tools.py Outdated Show resolved Hide resolved
@dashmage
Copy link
Contributor Author

dashmage commented Oct 3, 2023

Added unit tests for new function.

Copy link

@Pjack Pjack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sudeephb sudeephb left a comment

Choose a reason for hiding this comment

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

LGTM

@dashmage dashmage requested a review from jneo8 October 5, 2023 07:14
@dashmage dashmage merged commit 746e227 into canonical:master Oct 5, 2023
4 checks passed
@dashmage dashmage deleted the modify-redfish-discover branch October 5, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace discover with redfish basic login.
4 participants