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: move snap services to separate manager & monitor #217

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

st3v3nmw
Copy link
Collaborator

@st3v3nmw st3v3nmw commented Feb 23, 2024

Makes sure the extra services field doesn't trip up the server after the next landscape server release

@Perfect5th
Copy link
Contributor

We need to be a bit careful here. Releasing this client version without backporting the fix to still-supported server versions will break those server versions.

I'd like to suggest a non-backward-breaking option instead: we use a different message type for snap services. Server automatically ignores message types it isn't aware of, and that way reporting installed snaps will still work even if reporting services doesn't.

The new message type can still be sent by SnapMonitor, or we could introduce a separate monitor/manager for services, which may give us flexibility if we find that we need service-specific reporting cadence or data handling.

@st3v3nmw st3v3nmw marked this pull request as draft February 26, 2024 07:00
@st3v3nmw st3v3nmw changed the title fix: mark snap message as non-strict refactor: move snap services to separate manager & monitor Feb 26, 2024
@st3v3nmw st3v3nmw force-pushed the make-snap-msg-non-strict branch 6 times, most recently from 6b9a1e7 to 6bdb14f Compare February 26, 2024 12:35
@st3v3nmw
Copy link
Collaborator Author

st3v3nmw commented Feb 26, 2024

I've refactored the code to introduce a separate manager and monitor set for snap services (SnapServicesManager & SnapServicesMonitor).

Also fixed the snaps message in SnapManager._send_snap_update to match the outgoing message schema. The list of snaps was being attached to snaps instead of snaps.installed. This also matches the behavior of SnapMonitor

Tested that reporting of installed snaps is working as before and that the new SnapServicesManager & SnapServicesMonitor are registered when the client starts.

@st3v3nmw st3v3nmw force-pushed the make-snap-msg-non-strict branch 2 times, most recently from 0688297 to a9fd015 Compare February 26, 2024 13:57
@st3v3nmw st3v3nmw marked this pull request as ready for review February 26, 2024 14:57
Copy link
Contributor

@Perfect5th Perfect5th left a comment

Choose a reason for hiding this comment

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

LGMT, thanks!

@Perfect5th
Copy link
Contributor

Also tested against current version of landscape-server. Seems good.

@Perfect5th Perfect5th merged commit 53725b1 into canonical:master Feb 27, 2024
5 checks passed
@st3v3nmw st3v3nmw deleted the make-snap-msg-non-strict branch February 28, 2024 07:00
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