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

Store tools as a set of strings #365

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

aieri
Copy link
Contributor

@aieri aieri commented Dec 10, 2024

As noted in get_stored_tools(), StoredState cannot store arbitrary objects; this commit applies the same logic to _on_redetect_hardware()

Note: it would be nice to turn stored_tools into a property, but that requires a lot of unit test updates.

Fixes: #364

As noted the get_stored_tools(), StoredState cannot store arbitrary
objects; this commit applies the same logic to _on_redetect_hardware()

Fixes: canonical#364
@aieri aieri force-pushed the SOLENG-980-fix-redetect-hardware-apply branch from 9c66614 to 8102b2a Compare December 10, 2024 23:03
@aieri aieri marked this pull request as ready for review December 11, 2024 01:01
@aieri aieri requested a review from a team as a code owner December 11, 2024 01:01
@jneo8
Copy link
Contributor

jneo8 commented Dec 11, 2024

The type stored in _stored must be an instance of StoredObject, as outlined in the source code.

Since we're using the HWTool object as our model, we need to convert it into a set of strings. This conversion ensures it can be serialized and properly stored in SQLite.

Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

Maybe it's worth considering having a update_stored_tool wrapper function, since this line is used in two places

@aieri
Copy link
Contributor Author

aieri commented Dec 11, 2024

Maybe it's worth considering having a update_stored_tool wrapper function, since this line is used in two places

yes, that's what led me to try to turn get_stored_tools() into the getter of a stored_tools property (with self._stored.stored_tools = {tool.value for tool in available_tools} being the setter). Two hours and 100+ lines of unit test changes later and I'm not yet done :'(
I find this charm's tests to be very hard to understand.

@aieri aieri merged commit f8afe0e into canonical:main Dec 11, 2024
10 checks passed
@aieri aieri deleted the SOLENG-980-fix-redetect-hardware-apply branch December 11, 2024 02:42
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.

redetect-hardware action fails to save state if apply=true
3 participants