Skip to content
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

specifications: v1 history to v2 releases #3304

Merged
merged 8 commits into from Oct 5, 2020

Conversation

sergiusens
Copy link
Collaborator

Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

Internal migration from the Snap Store v1 history API endpoint to the
improved v2 releases endpoint which uses a /snap name/ instead of
=snap-id= as en entry point and returns a much more versatile dataset to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/en/an


On output, the following changes shall be made:

- Acknowledge that a Snap Revision can be compromised of multiple
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/compromised/comprimsed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Acknowledge" is an odd start. Given the opening, I'd suggest starting with the change(s), then provide rationale as needed. e.g. /Arch/ column renamed to /Arches/, to allow for a comma separated list as a snap revision and span multiple architectures.

- Acknowledge that a Snap Revision can be compromised of multiple
architectures, changing the output of the table from /Arch/ to /Arches/
and allowing for a comma separated list.
- Always displaying the full channel under /Channels/ (the history
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/displaying/display

There is a mix of future and present tense throughout.

and allowing for a comma separated list.
- Always displaying the full channel under /Channels/ (the history
endpoint return only <risk> for defaults).
- The /Channels/ comma separated list shall have no spaces for easier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: hypenate "comma-separated" or "comma-deliminated".


* Implementation

The Snap Store API for [[https://dashboard.snapcraft.io/docs/v2/en/snaps.html#snap-releases][Releases]] returns two /streams/ of interest, those
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M-x fill-pargraph 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did, the link gets to you :-P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh


* Implementation

The Snap Store API for [[https://dashboard.snapcraft.io/docs/v2/en/snaps.html#snap-releases][Releases]] returns two /streams/ of interest, those
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/of interest, those are.../of interest: =revisions= and =releases=.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using the suggestion :-)

* Implementation

The Snap Store API for [[https://dashboard.snapcraft.io/docs/v2/en/snaps.html#snap-releases][Releases]] returns two /streams/ of interest, those
are =revisions= and =releases=. These streams are ordered and relied on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ordered how? I don't think you need to state that it's relied on (anything in the API must be reliable).


And this is how the new output shall display:

#+BEGIN_SRC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example should probably capture all of the changes (e.g. multi-arch). And maybe used a more contrived example to keep it succinct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use a real world example to not make things up and for proper verification, I guess use made up data if that is desired


#+END_SRC

And this is how the new output shall display:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "And this is how" is very informal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the formal suggestion?


- [ ] Add bindings to Snap Store package
- [ ] Replace use of history with releases bindings for CLI
- [ ] Remove v1 history bindings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine there'd be a todo item for changing the output format? or is that coupled with history->release bindings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implied it with CLI, second item

@sergiusens sergiusens merged commit 0ae7c04 into canonical:master Oct 5, 2020
@sergiusens sergiusens deleted the history2releases-spec branch October 5, 2020 20:33
abitrolly pushed a commit to abitrolly/snapcraft that referenced this pull request Mar 31, 2021
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants