snap: add new type "TypeSnapd" and attach to the snapd snap #5704
Conversation
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
We use types sort sorting but the snapd snap is handled special (in the sort code and in more places). By adding a new "TypeSnapd" snap type this becomes more uniform. The current approach is to assign the type when reading the snap.yaml. This is mostly to make the transition easier for people who have snapd installed already but the snapd does not have the right type yet. We could omit it given that we have no real core18 systems in the wild yet and that snapd is not installable yet on classic/core16 systems.
zyga
approved these changes
Aug 22, 2018
snap/info.go
Outdated
| func (r ByType) Len() int { return len(r) } | ||
| func (r ByType) Swap(i, j int) { r[i], r[j] = r[j], r[i] } | ||
| func (r ByType) Less(i, j int) bool { | ||
| return typeOrder[r[i].Type] < typeOrder[r[j].Type] |
snap/types.go
Outdated
| // This is the sort order from least important to most important for | ||
| // types. On e.g. firstboot this will be used to order the snaps this | ||
| // way. | ||
| var typeOrder = map[Type]int{ |
There was a problem hiding this comment.
While this is perfectly fine I was thinking about something like this:
func (t Type) sortOrder() int {
return typeOrder[t]
}
snap/info.go
Outdated
| func (r ByType) Len() int { return len(r) } | ||
| func (r ByType) Swap(i, j int) { r[i], r[j] = r[j], r[i] } | ||
| func (r ByType) Less(i, j int) bool { | ||
| return typeOrder[r[i].Type] < typeOrder[r[j].Type] |
There was a problem hiding this comment.
If you would be interested in taking my advice below this then becomes r[i].Type.sortOrder() , r[j].Type.sortOrder()
| c.Assert(err, IsNil) | ||
| c.Assert(info.InstanceName(), Equals, "snapd") | ||
| c.Assert(info.Version, Equals, "1.0") | ||
| c.Assert(info.Type, Equals, snap.TypeSnapd) |
zyga
approved these changes
Aug 22, 2018
|
This failed on something weird: I've restarted the test but I suspect it will be "back". |
pedronis
reviewed
Aug 23, 2018
snapcraft.yaml
Outdated
| @@ -11,6 +11,7 @@ version: set-by-version-script-thxbye | |||
| version-script: | | |||
| ./mkversion.sh --output-only | |||
| grade: devel | |||
| type: snapd | |||
There was a problem hiding this comment.
we should ask review-tools and the store to add this new type
Codecov Report
@@ Coverage Diff @@
## master #5704 +/- ##
==========================================
+ Coverage 78.97% 78.98% +<.01%
==========================================
Files 524 524
Lines 40003 40003
==========================================
+ Hits 31594 31596 +2
+ Misses 5841 5839 -2
Partials 2568 2568
Continue to review full report at Codecov.
|
bboozzoo
reviewed
Aug 24, 2018
snap/info.go
Outdated
| return false | ||
| } | ||
| return typeOrder[r[i].Type] < typeOrder[r[j].Type] | ||
| return r[i].Type.sortOrder() < r[j].Type.sortOrder() |
There was a problem hiding this comment.
I'd capture this in a helper of Type so that the caller does not need to know if sorting order is low -> hi or hi -> low.
func (m Type) SortsBefore(other Type) bool {
return m.sortOrder() < m.sortOrder()
}
Then the code here becomes:
func (r ByType) Less(i, j int) bool {
return r[i].Type.SortsBefore(r[j].Type)
}
niemeyer
approved these changes
Aug 27, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A bit of an RFC - but I think we want this as it makes sorting easier
and once we have the type we can cleanup a bunch more.
We use types sort sorting but the snapd snap is handled special
(in the sort code and in more places). By adding a new "TypeSnapd"
snap type this becomes more uniform.
The current approach is to assign the type when reading the
snap.yaml. This is mostly to make the transition easier for
people who have snapd installed already but the snapd does
not have the right type yet. We could omit it given that we
have no real core18 systems in the wild yet and that snapd
is not installable yet on classic/core16 systems.
Based on #5702