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

logs: Do not use duplicate object to keep state #20137

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

marusak
Copy link
Member

@marusak marusak commented Mar 5, 2024

The source of the truth should be currentIdentifiers as defined in logs.jsx.
logsJournal.jsx had separate object to keep the same information and
than this currentIdentifiers was being synced. This was not ideal
solution which might fail when there are multiple updates at the same
time as well as it might not trigger update correctly when just editing
set object.

This commit only uses one source of the truth and uses updater callback
to make sure the list is always correctly updated.

The source of the truth should be `currentIdentifiers` as defined in `logs.jsx`.
`logsJournal.jsx` had separate object to keep the same information and
than this `currentIdentifiers` was being synced. This was not ideal
solution which might fail when there are multiple updates at the same
time as well as it might not trigger update correctly when just editing
set object.

This commit only uses one source of the truth and uses updater callback
to make sure the list is always correctly updated.
@marusak marusak force-pushed the logs_updater branch 2 times, most recently from 1f10e69 to d957bb9 Compare March 5, 2024 10:54
@marusak marusak marked this pull request as ready for review March 5, 2024 10:55
@marusak marusak requested review from jelly and martinpitt March 5, 2024 13:24
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Yay, thanks for debugging this! No alternative truths 😁

@marusak marusak merged commit 58ff0ef into cockpit-project:main Mar 6, 2024
74 checks passed
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.

2 participants