fix(snap): handle revision as str

[gboutry]

Fix #91

Snap revisions can be strings and should be handled as strings

[gboutry]

The ensure method could be modified to only accept int as revisions to enforce in the public api the installation of valid revisions. (disallow dev revisions such as x1)

[benhoyt]

I don't think this can go in as is, because it's a breaking change to change this field from an int to a str. So it would have to be a v2 of the charm lib if anything. But I'm not sure it's worth a v2 just for that change.

What's an "x" version? Does the "x" prefix mean local snap? Is the number after the "x" always 1, or does it increment? One possibility, especially if it's always "x1", would be to make "x1" map to version integer -1. I'm not sure what's best here -- I'd be interested to learn more about what the "x" means and when it happens.

[gboutry]

That's what I thought at first, but it was a str before, and the library version went from 11 to 12 with this change (meaning that at version 11 there was no issue with dev versions like x1)

[gboutry]

I'm not against keeping the external facing API's as int, but currently this library is making a charm fail because another snap installed on the host has x1 as version. (It's not the snap that the charm is managing)

[benhoyt]

Is a "dev version" a local .snap file? Are they always labelled "x1", or are there "x2" and "x3" and so on as well? What do you think of the idea to number dev versions with revision -1?

[gboutry]

Yeah what I meant by "dev version" is local revision. That would solve the problem, assuming we don't want to be able to address local revisions through the library

[benhoyt]

Hmm, I'm not sure what's best to do here. Do we need to be able to distinguish different local revisions such as x1 and x2? If not, I lean towards parsing revision "xN" to -1. Alternatively, for backwards compatibility, we could make the revision either an int or a str: it would be an int normally, but for xN revisions it would be left as a str. Or we could bump the lib to v2 and change it to str (but do revisions need to be compared numerically?).

It was in https://github.com/canonical/operator-libs-linux/pull/88 where we changed from str to int previously (in a minor version, which doesn't seem great to me, as ints and strs behave very differently -- though clearly it hasn't changed caused major problems).

So, summarizing the alternatives:

1) Parse local revisions with the "x" prefix as the integer -1. 2) Make revision an int unless it's an "xN" revision, in which case make it a str. 3) Make the revision always a str again, and bump the lib to v2.

I lean to number 1 to keep things simple. Thoughts, @jnsgruk?

[gboutry]

I don't believe we would want to manage local revision through a charm (there might be use case I don't see), so 1 is ok with me

[wolsen]

Currently, if a snap is installed locally (e.g. a dev snap) it will definitely have the xN number per the snap revision. It's clearly a string that is accepted, though it can be expected to be an integer for any snaps installed from the store.

It follows the specific path that @benhoyt suggested, if less than 0 then return x%d. Ben's suggestion is almost literally do the opposite: if start with x, then return -1. If we go that way, we could simply reverse the logic and do the opposite. However, I think in reality, the correct data type is a string as that is what the snap API is returning (and clearly it returns non-integers). While it is an integer in most cases (today), there's potential for the snap api/store api to change what the revision actually is (i.e. change from an int to a hash, etc).

As such, I think we should just bump the api version and change the data type back to the string type. Arguably, since the major version had it as a string originally and it was changed in a minor - it could be just switched back at a minor level as well. I don't think that's good practice though as it may be a breaking API change for consumers.

(as an aside, I did find it amusing that the integer is quoted like a string for the snap command line that ultimately gets executed)

[benhoyt]

Thanks @wolsen. Yeah, I think you're instinct is probably right. It is a string in the snapd API, so we should stick with that.

@gboutry Can you please update this PR to basically be what you have, but do so in a v2 version of the lib? I don't think this kind of change should have been done in a minor bump before, and two wrongs don't make a right. :-)

[gboutry]

Thanks @wolsen, @benhoyt

I updated to v2

[gboutry]

Hi @benhoyt, do you have an idea when we can get this published to charmhub ? thank you for you response

[jnsgruk]

@gboutry I just published this manually - it looks like our CI couldn't handle changing the major version.

@PietroPasotti I think you wrote the original lib publishing workflow, it seems here it didn't pick up the major version bump.

[gboutry]

Thank you @jnsgruk