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

Report which apps require update in DownloadStarted event #357

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

detsch
Copy link
Member

@detsch detsch commented Aug 28, 2024

This is what the DonwloadStarted event details look like for an "apps sync" update operation:

Syncing Active Target Apps
- shellhttpd2: missing installation, to be re-installed

(updated the above example after the latest force push)

I've started by only adding the app name, but decided to go a little further by adding the specific reason as well, so I created a new patch on top of the original implementation.

@detsch detsch requested a review from mike-sul August 28, 2024 20:28
src/aklite_client_ext.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
@mike-sul
Copy link
Contributor

@detsch This is the nice idea, thanks for doing it.

@detsch detsch force-pushed the detsch-improve-apps-sync-reason branch from 29201e6 to 4ce9c58 Compare August 29, 2024 20:00
@detsch
Copy link
Member Author

detsch commented Aug 29, 2024

I've updated the internal structure used to store the apps and reasons from vector to map, so the format is fully applied when building the reason string, not concatenating app name and reason before that.

@detsch
Copy link
Member Author

detsch commented Aug 29, 2024

@mike-sul I've taken a look into adding apps to update + reasons to the "update to target N+1" event, but it is not as trivial as this PR. We currently send the DownloadStarted event before knowing which apps need to be updated, and the code structure there is more complex, a lot more changes would be needed. I'll keep an eye on it and if I visualize some change that makes sense in that direction, I may try something. But it is not worth to deal with it in this PR.

src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
src/composeappmanager.cc Outdated Show resolved Hide resolved
@mike-sul
Copy link
Contributor

I'll keep an eye on it and if I visualize some change that makes sense in that direction, I may try something. But it is not worth to deal with it in this PR.

No need to worry about it. It is not so important for the regular update case (N to N+1 update).

This affects aklite CLI pull, install, and update commands.

Signed-off-by: Andre Detsch <[email protected]>
@detsch detsch force-pushed the detsch-improve-apps-sync-reason branch from 4ce9c58 to 91d67d8 Compare September 3, 2024 12:58
Adds additional information to the DownloadStarted event detailing the
reasons for an Apps Sync update to be initiated.

Signed-off-by: Andre Detsch <[email protected]>
@detsch detsch force-pushed the detsch-improve-apps-sync-reason branch from 91d67d8 to 05b018d Compare September 5, 2024 13:27
@detsch detsch merged commit 0ae576c into master Sep 5, 2024
4 checks passed
@detsch detsch deleted the detsch-improve-apps-sync-reason branch September 5, 2024 18:59
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