feat: track snap version#165
Conversation
james-garner-canonical
left a comment
There was a problem hiding this comment.
Hi there, thanks for your contribution. The changes seem broadly reasonable, but there are a couple of changes that would be necessary before merging.
Firstly, let's validate this change in integration tests -- just checking that the exposed version field contains a string as expected in one of the existing tests would be sufficient.
Secondly, adding a new, required argument to the public Snap class is not a backwards compatible change, particularly when added in the middle of other positional arguments. See separate comment for suggested resolution.
Also, I wonder if version is always guaranteed to be populated, or if it's up to the developer of the snap -- let's be defensive here.
Oh and if you could update the PR description with a description of the changes and the motivation, that would be great, thanks!
|
PS: don't forget to sign the CLA. EDIT: actually, what's going on with the |
|
Thank you very much for the review. I hope that my second commit answers all your suggestions. My initial understanding is that the version is something guaranteed to be required, but maybe you are right that version None can sometimes be allowed. I will shortly update the PR description with some content about the introduction of |
james-garner-canonical
left a comment
There was a problem hiding this comment.
Looks great, thanks!
Keep track of the snap version, if such version is available so that it could be used from a charm to set the workload version.
For example, an operator charm could set the version like this: