Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
overlord/snapstate: introduce tasks for aliases v2 semantics with temporary names for now (aliases v2) #3087
Conversation
pedronis
added
the
Critical
label
Mar 27, 2017
|
based on #3082 |
pedronis
changed the title from
overlord/snapstate: introduce tasks for aliases v2 semantics with temporary names for now
to
overlord/snapstate: introduce tasks for aliases v2 semantics with temporary names for now (aliases v2)
Mar 27, 2017
pedronis
closed this
Apr 2, 2017
pedronis
reopened this
Apr 3, 2017
pedronis
referenced this pull request
Apr 4, 2017
Merged
overlord: switch to aliases v2 tasks for install/refresh etc ops plus transition #3137
| + EnabledAliases AliasesStatus = "enabled" | ||
| + DisabledAliases AliasesStatus = "disabled" | ||
| + PendingEnabledAliases AliasesStatus = "pending:enabled" | ||
| + PendingDisabledAliases AliasesStatus = "pending:disabled" |
niemeyer
Apr 7, 2017
Contributor
I think the bad smell you found here comes from us merging what are actually two statuses in a single one:
- Do we want aliases enabled for this snap? (
AliasesEnabled bool, maybe) - Is there work pending to put that desired status in place? (
AliasesPending bool, maybe)
That removes the need for all these constants and all of the methods below, so seems like a clear win. As another minor benefit, AliasesPending will disappear in most cases due to omitempty.
Then, there's that fourth curious status which I don't yet see where it comes from. Reading on...
pedronis
Apr 7, 2017
Contributor
We discussed to indeed switch this to AliasesPending and AutoAliasesDisabled (both bool flags)
| + return false | ||
| +} | ||
| + | ||
| +// NoAutoYet return whether the status indicates no automatic aliases were introduced yet for the snap. |
niemeyer
Apr 7, 2017
Contributor
Do we really need that status? It feels like it should be something to compute dynamically just looking at the Enabled status plus the Aliases we currently have at hand. This would also make the logic slightly easier to read.
| + newAliases[alias] = &AliasTarget{Auto: curTarget.Auto} | ||
| + } | ||
| + } | ||
| + return curStatus.ToDisabled(), newAliases |
niemeyer
Apr 7, 2017
Contributor
Assuming suggestions above, that first bool might go since we can simply set Enabled to false.
| + return curStatus.ToDisabled(), newAliases | ||
| +} | ||
| + | ||
| +// pruneAutoAliases returns newAliases by dropping the automatic aliases autoAliases from curAliases, used as the task prune-auto-aliases to handle transfers of automatic aliases in a refresh. |
niemeyer
Apr 7, 2017
Contributor
I wonder if we shouldn't drop the manual aliases in those cases as well. Not something to worry too much about right now, though. We can easily tune the behavior depending on experience and user reaction.
| + return err | ||
| + } | ||
| + if status.NoAutoYet() && nAuto != 0 { | ||
| + status = PendingEnabledAliases |
niemeyer
Apr 7, 2017
Contributor
Shouldn´t that be done every time there are new auto aliases, rather than just when going from zero to something?
In either case, per notes above I think we might build the above conditional just looking at the data, instead of requiring a stored state. But perhaps I'm missing something.
| + if err != nil { | ||
| + return err | ||
| + } | ||
| + if status.NoAutoYet() && nAuto != 0 { |
| + snapst.Aliases = newAliases | ||
| + Set(st, snapName, snapst) | ||
| + return nil | ||
| +} |
niemeyer
Apr 7, 2017
Contributor
These task handlers are super readable despite the complexity of the problem.
Thanks for pulling that off!
pedronis
added some commits
Mar 24, 2017
Some checks were not successful
Some checks were not successful
All checks have passed
| - if len(a.Aliases) == 0 { | ||
| - return BadRequest("at least one alias name is required") | ||
| + if len(a.Aliases) != 0 { | ||
| + return BadRequest("cannot interpret request, snaps can no longer be expected to declare their aliases") |
pedronis
Apr 9, 2017
Contributor
yes, one of the next PRs needs to bring some form of this API back but probably we still need to use different fields or type, and be able to get a clean error for the previous usage
| + return newAliases | ||
| +} | ||
| + | ||
| +// pruneAutoAliases returns newAliases by dropping the automatic aliases autoAliases from curAliases, used as the task prune-auto-aliases to handle transfers of automatic aliases in a refresh. |
niemeyer
Apr 8, 2017
Contributor
Per our call this week, I think it'd make sense to disable manual aliases in this case as well. It's not a big deal to move it forward like this if you'd prefer, since we can easily tune the behavior later as we learn more, but it feels slightly surprising to have only the auto ones disabled.
pedronis
Apr 9, 2017
Contributor
I think there is a bit of confusion here, this is not for "prefer" and friends, "prefer" will indeed remove also manual aliases . This is for a corner case that we need to support though (in the old world we were using the task for snap alias --reset for this), this is about automatic aliases that go from being exposed from snap A to being exposed by another snap B (to be realistic this would probably involve related snaps or some snap being split for some reason)
| @@ -819,3 +819,194 @@ func (m *SnapManager) doDiscardSnap(t *state.Task, _ *tomb.Tomb) error { | ||
| Set(st, snapsup.Name(), snapst) | ||
| return nil | ||
| } | ||
| + | ||
| +// aliases v2 |
niemeyer
Apr 8, 2017
Contributor
It would be great to have here a comment explaining why we have the multi-step process below with multiple tasks, and why we need the pending flag, as this will definitely feel a bit surprising and confusing on a first read.
It's okay to do that on a follow up though.
pedronis
added this to the 2.25 milestone
Apr 11, 2017
pedronis
added some commits
Apr 12, 2017
Some checks were not successful
All checks have passed
| + return newAliases | ||
| +} | ||
| + | ||
| +// pruneAutoAliases returns newAliases by dropping the automatic aliases autoAliases from curAliases, used as the task prune-auto-aliases to handle transfers of automatic aliases in a refresh. |
niemeyer
Apr 8, 2017
Contributor
Per our call this week, I think it'd make sense to disable manual aliases in this case as well. It's not a big deal to move it forward like this if you'd prefer, since we can easily tune the behavior later as we learn more, but it feels slightly surprising to have only the auto ones disabled.
pedronis
Apr 9, 2017
Contributor
I think there is a bit of confusion here, this is not for "prefer" and friends, "prefer" will indeed remove also manual aliases . This is for a corner case that we need to support though (in the old world we were using the task for snap alias --reset for this), this is about automatic aliases that go from being exposed from snap A to being exposed by another snap B (to be realistic this would probably involve related snaps or some snap being split for some reason)
pedronis commentedMar 27, 2017
•
Edited 1 time
-
pedronis
Mar 27, 2017
This introduces new tasks for the aliases v2 semantics. They are added with temporary "*-v2" names for now, they are not used in changes yet but can be tested this way.
Next couple of branches will do the switch over, migration and take care of most TODOs here except for implementing --prefer etc that will come after the basics are in place.
The old aliases could reuse the "alias" task "reset" mode to support changes to automatic aliases independent of a new revision of the snap on refresh, for the new world this introduces "refresh-aliases" and "drop-auto-aliases" for those situations.
This will be relatively large even after the prereqs are merged but much more than half of this are tests.
I'm not entirely happy about the logic handling "Pending" and AliasesStatus, open to suggestions there.