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

Remove unnecessary use of StoreState for configs #209

Merged

Conversation

chanchiwai-ray
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray commented Mar 28, 2024

In config changed event, we should not keep the configs in StoreState just for tracking the which config option is changed, because any change of config option will result in a re-render of the exporter config yaml. The condition is almost always truthy so the tracking mechanism is useless.

Closes #176, closes #208

@chanchiwai-ray chanchiwai-ray added the enhancement New feature or request label Mar 28, 2024
@chanchiwai-ray chanchiwai-ray self-assigned this Mar 28, 2024
@chanchiwai-ray chanchiwai-ray requested a review from a team as a code owner March 28, 2024 07:42
@chanchiwai-ray
Copy link
Contributor Author

chanchiwai-ray commented Mar 28, 2024

Will check if manual tests are required, marking WIP

@chanchiwai-ray
Copy link
Contributor Author

These changes should not require a manual test since it almost did not touch the core logic in the charm. Marking it "ready to review" by removing "[WIP]". Feel free to let me know if you want to manually test the charm in some way.

@chanchiwai-ray chanchiwai-ray changed the title [WIP] Remove unnecessary use of StoreState for configs Remove unnecessary use of StoreState for configs Apr 2, 2024
src/charm.py Show resolved Hide resolved
jneo8
jneo8 previously approved these changes Apr 3, 2024
src/charm.py Outdated Show resolved Hide resolved
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
Left comment on just a small typo

tests/unit/test_charm.py Outdated Show resolved Hide resolved
sudeephb
sudeephb previously approved these changes Apr 5, 2024
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@rgildein rgildein left a comment

Choose a reason for hiding this comment

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

Thanks, please add issue you reported in comment here or mention that comment in the issue.

@chanchiwai-ray
Copy link
Contributor Author

Open an issue found in this PR: #215

@chanchiwai-ray chanchiwai-ray merged commit 4a1b809 into canonical:master Apr 5, 2024
5 checks passed
@chanchiwai-ray chanchiwai-ray deleted the issue/176/beseng-2090 branch April 5, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants