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

refactor: Collect all status in one place #195

Closed
wants to merge 2 commits into from

Conversation

dashmage
Copy link
Contributor

  • Set all statuses from the collect_unit_status event handler.
  • Change name from exporter_enabled to relation_present
  • Refactor code into render_exporter_config
  • Change return format for validate_exporter_configs to be simpler.

@dashmage
Copy link
Contributor Author

A lot of unit tests break with this change. Once the overall logic is finalized, I'll go ahead and make the required unit test modifications.

@dashmage dashmage self-assigned this Mar 25, 2024
@dashmage
Copy link
Contributor Author

dashmage commented Mar 25, 2024

I'm thinking whether I should make changes to resolve #201 as well since I'm anyway changing the stored state variables with this PR.

Comment on lines -77 to -78
exporter_installed=False,
resource_installed=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better that we avoid using store for *_installed here, and create methods to directly verify them

* Set all statuses from the collect_unit_status event handler.
* Change name from exporter_enabled to relation_present
* Refactor code into render_exporter_config
* Change return format for validate_exporter_configs to be simpler.
@dashmage dashmage force-pushed the bseng-2199-collect-status branch from 3f0f931 to 24bf362 Compare March 27, 2024 12:02
@Pjack
Copy link

Pjack commented Jun 19, 2024

Close this PR, since it was pending for a while.

@Pjack Pjack closed this Jun 19, 2024
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.

3 participants