daemon, store: switch to new store APIs in snapd #2036

pete-woods · 2016-09-29T18:35:20Z

No description provided.

niemeyer

Looks okayish. Seems fine to merge after a few tweaks.

The surroundings of this logic are becoming a bit debt-loaded, though. Orders, purchases, buy.. feels like the underlying idea is pretty simple, and we might be over-complicating it a bit on terminology overload.

Can we look at cleaning this up after the core feature is working?

niemeyer · 2016-09-29T19:53:19Z

store/store.go

@@ -664,66 +664,58 @@ func (s *Store) extractSuggestedCurrency(resp *http.Response) {
 	}
 }

-// purchase encapsulates the purchase data sent to us from the software center agent.
+// order encapsulates the purchase data sent to us from the software center agent.


Doc isn't matching real type name.

Can we please call this ordersResult, to avoid orders.Orders?

niemeyer · 2016-09-29T19:58:42Z

store/store.go

-	purchasesByID := make(map[string][]*purchase)
-	for _, purchase := range purchases {
-		purchasesByID[purchase.SnapID] = append(purchasesByID[purchase.SnapID], purchase)
+	purchasesByID := make(map[string]bool)


Comment and variable name seem wrong now, and logic itself is also outdated. It's building a map of all the possible snap ids just to check whether one of them was bought. We don't need a map for that.

John: After discussion, we resolved this misunderstanding, and agreed on naming for the map.

pete-woods · 2016-09-29T20:49:20Z

I definitely agree re the debt. I was trying to keep the diff to as readable a level as possible, as there are a lot of little details that could have been easily missed when making these changes. There are a few follow up branches after this, to do things like clean out the /v2/buy/methods endpoint, at very least.

- rename order->orderResult - rename purchased->bought

- rename map ordersByID->bought

chipaca

This is OK. Feel free to ignore the nits, but please add the comment around the spurious ReadyToBuy call. Feel free to quote me verbatim if you want, or be more polite if you need to.

chipaca · 2016-09-30T08:21:28Z

daemon/api.go

@@ -1782,6 +1782,44 @@ func postCreateUser(c *Command, r *http.Request, user *auth.UserState) Response
 	}, nil)
 }

+func convertBuyError(err error) Response {
+	switch err {
+	default:


nit: I'd rather default be the last case in a switch

chipaca · 2016-09-30T08:22:53Z

daemon/api.go

+	case store.ErrInvalidCredentials:
+		return Unauthorized(err.Error())
+	case store.ErrUnauthenticated:
+		return SyncResponse(&resp{


could you add helper so these all become a return BadRequestKind(kind, err)? See e.g. makeErrorResponder in response.go

(and yes, I know there already are a bunch of these, so if you want to leave it I'll do it in a branch all of its own)

agreed to fix this in a followup

chipaca · 2016-09-30T08:24:29Z

daemon/api.go

-		return Unauthorized(err.Error())
-	case nil:
-		// continue
+	resp := convertBuyError(err)


nit: you don't need resp outside the if, so move it into it

chipaca · 2016-09-30T08:57:48Z

store/store.go

+		return nil, ErrUnauthenticated
+	}
+
+	err := s.ReadyToBuy(user)


This is stupid and I hate it and it's a clear, glaring sign of a design failure (or documentation failure) of the purchase API. Having to do more than one RPC for this is just asking for a whole family of bugs and weird behaviours.

I understand that it is necessary for now, but please can you put a huge comment around it demanding we fix it?

chipaca · 2016-09-30T09:03:24Z

store/store.go

@@ -1581,7 +1525,7 @@ type storeCustomer struct {
 // ReadyToBuy returns nil if the user's account has accepted T&Cs and has a payment method registered, and an error otherwise
 func (s *Store) ReadyToBuy(user *auth.UserState) error {
 	if user == nil {
-		return ErrInvalidCredentials


chipaca · 2016-09-30T09:12:57Z

daemon/api.go

+			},
+			Status: http.StatusBadRequest,
+		}, nil)
+	case nil:


followup nit: in the spirit of "exit early", this is usually the first case in the switch

chipaca · 2016-09-30T09:13:21Z

daemon/api.go

-		}, nil)
-	case nil:
-		// continue
+	resp := convertBuyError(err)


ditto here wrt moving it into the if

- Re-order case statements. - Minimise scope of resp / err variables. - Add FIXME to ReadyToBuy call.

Imported using git-ubuntu import. Changelog parent: d92891f New changelog entries: * New upstream release, LP: #1637215: - release: os-release on core has changed - tests: /dev/ptmx does not work on powerpc, skip here - docs: moved to github.com/snapcore/snapd/wiki (snapcore#2258) - debian: golang is not installable on powerpc, use golang-any * New upstream release, LP: #1637215: - overlord/ifacestate: add unit tests for undo of setup-snap- security (snapcore#2243) - daemon,overlord,snap,tests: download to .partial in final dir (snapcore#2237) - overlord/state: marshaling tests for lanes (snapcore#2245) - overlord/state: introduce state lanes (snapcore#2241) - overlord/snapstate: fix revert+refresh (snapcore#2224) - interfaces/sytemd: enable/disable generated service units (snapcore#2229) - many: fix incorrect security files generation on undo - overlord/snapstate: add dynamic snapdX.Y assumes (snapcore#2227) - interfaces: network-manager: give slot full read-write access to /run/NetworkManager - docs: update the name of the command for the cross-build - overlord/snapstate: fix missing argument to Noticef - snapstate: ensure gadget/core/kernel can not be disabled (snapcore#2218) - asserts: limit to 1y only if len(models) == 0 (snapcore#2219) - debian: only install share/locale if available (missing on powerpc) - overlrod/snapstate: fix revert followed by refresh to old-current (snapcore#2214) - interfaces/builtin: network-manager and bluez can change hostname (snapcore#2204) - snap: switch the auto-import dir to /run/snapd/auto-import - docs: less details about cloud.cfg as requested in trello (snapcore#2206) - spread.yaml: Ensure ubuntu user has passwordless sudo for autopkgtests (snapcore#2201) - interfaces/builtin: add dcdbas-control interface - boot: do not set boot to try mode if the revision is unchanged - interfaces: add shutdown interface (snapcore#2162) - interfaces: add system-power-control interface - many: use the new systemd backend for configuring GPIOs - overlord/ifacestate: setup security for slots before plugs - snap: spool assertion candidates if snapd is not up yet - store,daemon,overlord: download things to a partials dir - asserts,daemon: implement system-user-authority header/concept - interfaces/builtin: home base declaration rule using on-classic for its policy - interfaces/builtin: finish decl based checks - asserts: bump snap-declaration to allow signing with new-style plugs and slots - overlord: checks for kernel installation/refresh based on model assertion and previous kernel - tests/lib/fakestore: fix logic to distinguish assertion not found errors - client: add a few explicit error types (around the request cycle) - tests/lib/fakestore/cmd/fakestore: make it log, and fix a typo - overlord/snapstate: two bugs for one - snappy: disable auto-import of assertions on classic (snapcore#2122) - overlord/snapstate: move trash cleanup to a cleanup handler (snapcore#2173) - daemon: make create-user --known fail on classic without --force- managed (snapcore#2123) - asserts,interfaces/policy: implement on-classic plug/slot constraints - overlord: check that the first installed gadget matches the model assertion - tests: use the snapd-control-consumer snap from the store - cmd/snap: make snap run not talk to snapd for finding the revision - snap/squashfs: try to hard link instead of copying. Also, switch to osutil.CopyFile for cp invocation. - store: send supported max-format when retrieving assertions - snapstate, devicestate: do not remove seed - boot,image,overlord,partition: read/write boot variables in single operation - tests: reenable ubuntu-core tests on qemu - asserts,interfaces/policy: allow OR-ing of subrule constraints in plug/slot rules - many: move from flags as ints to flags as structs-of-bools (snapcore#2156) - many: add supports for keeping and finding assertions with different format iterations - snap: stop using ubuntu-core-launcher, use snap-confine - many: introduce an assertion format iteration concept, refuse to add unsupported assertion - interfaces: tweak wording and comment - spread.yaml: dump apparmor denials on spread failure - tests: unflake ubuntu-core-reboot (snapcore#2150) - cmd/snap: tweak unknown command error message (snapcore#2139) - client,daemon,cmd: add payment-declined error kind (snapcore#2107) - cmd/snap: update remove command help (snapcore#2145) - many: removed frameworks target and fixed service files (snapcore#2138) - asserts,snap: validate attributes to a JSON-compatible type subset (snapcore#2140) - asserts: remove unused serial-proof type - tests: skip auto-import tests on systems without test keys (snapcore#2142) - overlord/devicestate: don't spam the debug log on classic (snapcore#2141) - cmd/snap: simplify auto-import mountinfo parsing (snapcore#2135) - tests: run ubuntu-core upgrades on isolated machine (snapcore#2137) - overlord/devicestate: recover seeding from old external approach (snapcore#2134) - overlord: merge overlord/boot pkg into overlord/devicestate (snapcore#2118) - daemon: add postCreateUserSuite test suite (snapcore#2124) - tests: abort tests if an update process is scheduled (snapcore#2119) - snapstate: avoid reboots if nothing in the boot setup has changed (snapcore#2117) - cmd/snap: do not auto-import from loop or non-dev devices (snapcore#2121) - tests: add spread test for `snap auto-import` (snapcore#2126) - tests: add test for auto-mount assertion import (snapcore#2127) - osutil: add missing unit tests for IsMounted (snapcore#2133) - tests: check for failure creating user on managed ubuntu-core systems (snapcore#2096) - snap: ignore /dev/loop addings from udev (snapcore#2111) - tests: remove snapd.boot-ok reference (snapcore#2109) - tests: enable tests related to the home interface in all-snaps (snapcore#2106) - snapstate: only import defaults from gadget on install (snapcore#2105) - many: move firstboot code into the snapd daemon (snapcore#2033) - store: send correct JSON type of string for expected payment amount (snapcore#2103) - cmd/snap: rename is-managed to managed and tune (snapcore#2102) - interfaces,overlord/ifacestate: initial cleaning up of no arg AutoConnect related bits (snapcore#2090) - client, cmd: prompt for password when buying (snapcore#2086) - snapstate: fix hanging `snap remove` if snap is no longer mounted - image: support gadget specific cloud.conf file (snapcore#2101) - cmd/snap,ctlcmd: fix behavior of snap(ctl) get (snapcore#2093) - store: local users download from the anonymous url (snapcore#2100) - docs/hooks.md: fix typos (snapcore#2099) - many: check installation of slots and plugs against declarations - docs: fix missing "=" in the systemd-active docs - store: do not set store auth for local users (snapcore#2092) - interfaces,overlord/ifacestate: use declaration-based checking for auto-connect (snapcore#2071) - overlord, daemon, snap: support gadget config defaults (snapcore#2082)The main semantic changes are: - tests: fix snap-disconnect tests after core rename (snapcore#2088) - client,daemon,overlord,cmd: add /v2/users and create-user on auto- import (snapcore#2074) - many: abbreviated forms of disconnect (snapcore#2066) - asserts: require lowercase model until insensitive matching is ready (snapcore#2076) - cmd/snap: add version command, same as --version (snapcore#2075) - all: use "core" by default but allow "ubuntu-core" still (snapcore#2070) - overlord/devicestate, docs/hooks.md: nest prepare-device configuration options - daemon: fix login API to return local macaroons (snapcore#2078) - daemon: do not hardcode UID in userLookup (snapcore#2080) - client, cmd: connect fixes (snapcore#2026) - many: preparations for switching most of autoconnect to use the declarationsfor now: - overlord/auth: update CheckMacaroon to verify local snapd macaroons (snapcore#2069) - cmd/snap: trivial auto-import and download tweaks (snapcore#2067) - interfaces: add repo.ResolveConnect that handles name resolution - interfaces/policy: introduce InstallCandidate and its checks - interfaces/policy,overlord: check connection requests against the declarations in ifacestate - many: setup snapd macaroon for local users (snapcore#2051)Next step: do snapd macaroons verification. - interfaces/policy: implement snap-id/publisher-id checks - many: change Connect to take ConnRef instead of strings (snapcore#2060) - snap: auto mount block devices and import assertions (snapcore#2047) - daemon: add `snap create-user --force-managed` support (snapcore#2041) - docs: remove references to removed buying features (snapcore#2057) - interfaces,docs: allow sharing SNAP{,_DATA,_COMMON} via content iface (snapcore#2063) - interfaces: add Plug/Slot/Connection reference helpers (snapcore#2056) - client,daemon,cmd/snap: improve create-user APIs (snapcore#2054) - many: introduce snap refresh --ignore-validation <snap> to override refresh validation (snapcore#2052) - daemon: add support for `snap create-user --known` (snapcore#2040) - interfaces/policy: start of interface policy checking code based on declarations (snapcore#2050) - overlord/configstate: support nested configuration (snapcore#2039) - asserts,interfaces/builtin,overlord/assertstate: introduce base- declaration (snapcore#2037) - interfaces: builtin: Allow writing DHCP lease files to /run/NetworkManager/dhcp (snapcore#2049) - many: remove all traces of the /v2/buy/methods endpoint (snapcore#2045) - tests: add external spread backend (snapcore#1918) - asserts: parse the slot rules in snap-declarations (snapcore#2035) - interfaces: allow read of /etc/ld.so.preload by default for armhf on series 16 (snapcore#2048) - store: change purchase to order and store clean up first pass (snapcore#2043) - daemon, store: switch to new store APIs in snapd (snapcore#2036) - many: add email to UserState (snapcore#2038) - asserts: support parsing the plugs stanza i.e. plug rules in snap- declarations (snapcore#2027) - store: apply deltas if explicitly enabled (snapcore#2031) - tests: fix create-key/snap-sign test isolation (snapcore#2032) - snap/implicit: don't restrict the camera iface to clasic (snapcore#2025) - client, cmd: change buy command to match UX document (snapcore#2011) - coreconfig: nuke it. Also, ignore po/snappy.pot. (snapcore#2030) - store: download deltas if explicitly enabled (snapcore#2017) - many: allow use of the system user assertion with create-user (snapcore#1990) - asserts,overlord,snap: add prepare-device hook for device registration (snapcore#2005) - debian: adjust packaging for trusty/deputy systemd (snapcore#2003) - asserts: introduce AttributeConstraints (snapcore#2015) - interface/builtin: access system bus on screen-inhibit-control - tests: add firewall-control interface test (snapcore#2009) - snapstate: pass errors from ListRefresh in updateInfo (snapcore#2018) - README: add links to IRC, mailing list and social media (snapcore#2022) - docs: add `configure` hook to hooks list (snapcore#2024)LP: #1596629 - cmd/snap,configstate: rename apply-config variables to configure. (snapcore#2023) - store: retry download on 500 (snapcore#2019) - interfaces/builtin: support time and date settings via 'org.freedesktop.timedate1 (snapcore#1832)

daemon, store: switch to new store APIs in snapd

c0759be

pete-woods force-pushed the snapd-use-new-store-purchase-apis branch from 384019f to c0759be Compare September 29, 2016 18:40

niemeyer approved these changes Sep 29, 2016

View reviewed changes

pete-woods added 2 commits September 29, 2016 22:02

respond to Gustavo's review comments

72060c0

- rename order->orderResult - rename purchased->bought

Respond to Gustavo's review comments

939213d

- rename map ordersByID->bought

chipaca approved these changes Sep 30, 2016

View reviewed changes

respond to John's review comments

24d55b6

- Re-order case statements. - Minimise scope of resp / err variables. - Add FIXME to ReadyToBuy call.

pete-woods merged commit 63eec5c into snapcore:master Sep 30, 2016

pete-woods deleted the snapd-use-new-store-purchase-apis branch October 5, 2016 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon, store: switch to new store APIs in snapd #2036

daemon, store: switch to new store APIs in snapd #2036

pete-woods commented Sep 29, 2016

niemeyer left a comment

niemeyer Sep 29, 2016 •

edited

pete-woods Sep 30, 2016

niemeyer Sep 29, 2016 •

edited

pete-woods Sep 30, 2016

pete-woods commented Sep 29, 2016

chipaca left a comment

chipaca Sep 30, 2016

chipaca Sep 30, 2016 •

edited

chipaca Sep 30, 2016

pete-woods Sep 30, 2016

chipaca Sep 30, 2016

chipaca Sep 30, 2016 •

edited

chipaca Sep 30, 2016

chipaca Sep 30, 2016

chipaca Sep 30, 2016

daemon, store: switch to new store APIs in snapd #2036

daemon, store: switch to new store APIs in snapd #2036

Conversation

pete-woods commented Sep 29, 2016

niemeyer left a comment

Choose a reason for hiding this comment

niemeyer Sep 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer Sep 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-woods commented Sep 29, 2016

chipaca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipaca Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer Sep 29, 2016 •

edited

niemeyer Sep 29, 2016 •

edited

chipaca Sep 30, 2016 •

edited

chipaca Sep 30, 2016 •

edited