Merge pull request 'Compose an account given only groups where its module wrote it (hq ADR 0252, issue 247)' (#139) from feat/an-account-only-given-groups-keeps-its-place into main

internal/catalogue/declaration.go

@@ -2396,9 +2396,15 @@ func withRestartOn(have any, add []string) []any {
 
 // accountsFirst splits a module's resources into its accounts and everything else, each in the order
 // written.
+//
+// **An account declared only to be put in groups keeps its written place** (novox/hq ADR 0252). It is not
+// the module's own account — that one says how it logs in or where it lives — but one the module needs in
+// a group: the operator's, in the group of a daemon the module installs. The group is made by the package
+// that installs the daemon, so put first, the account was put in a group the machine did not have yet, and
+// only the next apply healed it. Written after its packages, it is applied after them.
 func accountsFirst(resources []map[string]any) (accounts, rest []map[string]any) {
 	for _, r := range resources {
-		if fmt.Sprint(r["type"]) == "user" {
+		if fmt.Sprint(r["type"]) == "user" && !onlyGroups(r) {
 			accounts = append(accounts, r)
 			continue
 		}
@@ -2407,6 +2413,17 @@ func accountsFirst(resources []map[string]any) (accounts, rest []map[string]any)
 	return accounts, rest
 }
 
+// onlyGroups is a user resource that names groups and nothing else of the account: no shell, home or
+// lingering.
+func onlyGroups(r map[string]any) bool {
+	for _, field := range []string{"shell", "home", "linger"} {
+		if v, ok := r[field]; ok && v != nil && v != "" {
+			return false
+		}
+	}
+	return len(stringsIn(r["groups"])) > 0
+}
+
 // generatedHere is what each computed module's generator answers for this machine, by module —
 // absent for a module whose machine is not yet part of what it generates — held to the rule every
 // module is held to: no two modules on a machine declare one path, unit, name or package (novox/hq

internal/catalogue/resolve.go

@@ -949,9 +949,9 @@ func checkResources(modules []Manifest) []string {
 			if fmt.Sprint(r["type"]) == "user" {
 				// **An account is shared; what it is set to is not.** Several modules may need one
 				// login: the shell's module sets its shell, the container runtime's puts it in the
-				// `docker` group. The host only ever adds groups — it never takes the account out of
+				// `docker` group. The host only ever adds groups, and takes back only a group it put the
-				// one, not even when the resource that named it is undeclared — so groups from
+				// account in, when no declared resource still asks for it (novox/hq ADR 0252) — so groups
-				// several modules cannot contradict each other and are not owned. A shell or a home
+				// from several modules cannot contradict each other and are not owned. A shell or a home
 				// is one value, and two modules setting it would each be undone by the other's
 				// apply: each stays one module's per node, and two are refused naming both.
 				name, _ := r["name"].(string)

internal/catalogue/shared_account_test.go

@@ -40,3 +40,34 @@ func TestTwoModulesSettingOneAccountsShellOrHomeAreRefused(t *testing.T) {
 		}
 	}
 }
+
+// An account a module declares only to put in a group is applied where it is written, after the package
+// that makes the group (novox/hq ADR 0252); one that says how it logs in still goes first (issue 213).
+func TestAnAccountOnlyGivenGroupsKeepsItsWrittenPlace(t *testing.T) {
+	compose := func(raw string) []map[string]any {
+		t.Helper()
+		m, err := ParseManifest([]byte(raw))
+		if err != nil {
+			t.Fatal(err)
+		}
+		out, err := Resolution{Node: "workstation", Modules: []Manifest{m}}.Declaration(Rendering{})
+		if err != nil {
+			t.Fatal(err)
+		}
+		return out
+	}
+	out := compose(`{"module": "lights", "version": "1", "resources": [
+	  {"id": "daemon", "type": "package", "package": "lights-daemon"},
+	  {"id": "account", "type": "user", "name": "op", "groups": ["lights"]}
+	]}`)
+	if pkg, acct := indexOf(out, "lights.daemon"), indexOf(out, "lights.account"); pkg < 0 || acct < pkg {
+		t.Fatalf("the account (%d) is not after the package that makes its group (%d): %v", acct, pkg, out)
+	}
+	out = compose(`{"module": "shell", "version": "1", "resources": [
+	  {"id": "package", "type": "package", "package": "zsh"},
+	  {"id": "login", "type": "user", "name": "op", "shell": "/usr/bin/zsh", "groups": ["wheel"]}
+	]}`)
+	if pkg, acct := indexOf(out, "shell.package"), indexOf(out, "shell.login"); acct < 0 || acct > pkg {
+		t.Fatalf("an account with a shell is no longer first (%d, package %d): %v", acct, pkg, out)
+	}
+}