-
Notifications
You must be signed in to change notification settings - Fork 562
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
api: Add snap info to each task #13902
Closed
sergio-costas
wants to merge
1
commit into
snapcore:master
from
sergio-costas:UDENG-2282-add-snap-info-in-each-task
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to add this kind of data this way. We probably want to add support instead for something like api-data that we have for Changes. Also (even) if we do that the closest thing we can do that doesn't feel too forced would be to expose a list of instance names "affected-snaps" that matches the same concept in snapstate conflict logic, we would have to expose some of that to daemon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, how are the consumers expected to use this information? Say if a change touches more than one, would they list all the tasks, and then continue observing which ones for which snap are done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboozzoo consumer is expected to poll the associated refresh change, if it happens to include multiple snaps then filtering on the task level is needed.
Check the "Proposed snap-run during refresh UX flow" section in SD167.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see a video with what I have right now: https://warthogs.atlassian.net/browse/UDENG-2282 , in the most recent comment. As you can see, the idea is to have a progress bar for each snap being updated, and that requires to know how many tasks are assigned to that snap refresh, and how many are already done. Currently I'm presuming that there is one change per snap, so each change has only one snap in its snap_names property, and thus all the tasks belong to that snap. But if there are several snaps in the same change, then each progress bar will show the same progress, unless this patch (or something similar) is implemented, that allows me to separate the tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, here is an screen capture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unclear that "snap-tasks" is a good name for it, so we don't have a good name, also it's not an obvious approach except is compact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the obvious way to attach the information in snap-tasks form would be when we set snap-names, but actually a Change can grow more tasks after that point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point... if the task list can grow, then it makes sense to include this info in the task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to reiterate on this, snap can pull prerequisites (like a new base) or a re-refresh may occur, at which point the task list will grow again, so any progress bar the user sees will need to be adjusted such that it doesn't jump backwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still another scenario where
snap run
of a certain snap is inhibited from running due to a normal refresh/auto-refresh that includes that snap and the progress UI should only be relevant to the snap that is being started and not the other snaps in the auto-refresh. It not just when running the snap after closing.