-
Notifications
You must be signed in to change notification settings - Fork 2
Support reclaiming mannequins to customer-owned bot accounts #7
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
Changes from all commits
2c53a94
27b5513
e7278f6
70270ca
dbea179
675ba7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ package target | |
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
|
|
@@ -185,7 +186,6 @@ func TestMannequinReclaim(t *testing.T) { | |
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "aborted") | ||
| }) | ||
|
|
||
| t.Run("accepts the legacy claim command and flags", func(t *testing.T) { | ||
| _, err := runMannequin(t, newMannequinClaimCmd, "", | ||
| "--github-org", "octo", "--target-url", "https://x", "--target-token", "tok") | ||
|
|
@@ -214,4 +214,140 @@ func TestMannequinReclaim(t *testing.T) { | |
| assert.Contains(t, err.Error(), "no target URL configured") | ||
| assert.NotContains(t, err.Error(), "duplicates") | ||
| }) | ||
|
|
||
| t.Run("reclaims to a bot after confirmation", func(t *testing.T) { | ||
| srv, called := botClaimServer(t, "legacy-ci[bot]") | ||
| defer srv.Close() | ||
|
|
||
| out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", | ||
| "octo", "legacy-ci[bot]", "example-ci[bot]", | ||
| "--target-url", srv.URL, "--target-token", "tok") | ||
| require.NoErrorf(t, err, "claim output:\n%s", out) | ||
| assert.True(t, *called, "expected reattributeMannequinToBot to be called") | ||
| }) | ||
|
|
||
| t.Run("aborts a bot reclaim when the admin declines the prompt", func(t *testing.T) { | ||
| srv, called := botClaimServer(t, "legacy-ci[bot]") | ||
| defer srv.Close() | ||
|
|
||
| _, err := runMannequin(t, newMannequinReclaimCmd, "n\n", | ||
| "octo", "legacy-ci[bot]", "example-ci[bot]", | ||
| "--target-url", srv.URL, "--target-token", "tok") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "aborted") | ||
| assert.False(t, *called, "no reclaim should occur after declining") | ||
| }) | ||
|
|
||
| t.Run("bot reclaim with --no-prompt proceeds without confirmation", func(t *testing.T) { | ||
| srv, called := botClaimServer(t, "legacy-ci[bot]") | ||
| defer srv.Close() | ||
|
|
||
| out, err := runMannequin(t, newMannequinReclaimCmd, "", | ||
| "octo", "legacy-ci[bot]", "example-ci[bot]", | ||
| "--no-prompt", "--target-url", srv.URL, "--target-token", "tok") | ||
| require.NoErrorf(t, err, "claim output:\n%s", out) | ||
| assert.True(t, *called, "expected reattributeMannequinToBot to be called") | ||
| }) | ||
|
|
||
| t.Run("warns when the source mannequin does not look like a bot", func(t *testing.T) { | ||
| srv, called := botClaimServer(t, "alice") | ||
| defer srv.Close() | ||
|
|
||
| out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", | ||
| "octo", "alice", "example-ci[bot]", | ||
| "--target-url", srv.URL, "--target-token", "tok") | ||
| require.NoErrorf(t, err, "claim output:\n%s", out) | ||
| assert.True(t, *called, "expected reattributeMannequinToBot to be called") | ||
| assert.Contains(t, out, "does not look like a bot mannequin", "expected soft-signal advisory warning") | ||
| }) | ||
|
|
||
| t.Run("reclaims a bot row from a CSV with mixed targets", func(t *testing.T) { | ||
| srv, botCalled, invited := botCSVServer(t) | ||
| defer srv.Close() | ||
|
|
||
| path := filepath.Join(t.TempDir(), "mannequins.csv") | ||
| csv := "mannequin-user,mannequin-id,target-user\n" + | ||
| "legacy-ci[bot],m1,example-ci[bot]\n" + | ||
| "alice,m2,alice-t\n" | ||
| require.NoError(t, os.WriteFile(path, []byte(csv), 0o600)) | ||
|
|
||
| out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", | ||
| "octo", "--csv", path, | ||
| "--target-url", srv.URL, "--target-token", "tok") | ||
| require.NoErrorf(t, err, "claim output:\n%s", out) | ||
| assert.True(t, *botCalled, "expected reattributeMannequinToBot to be called for the bot row") | ||
| assert.True(t, *invited, "expected createAttributionInvitation to be called for the human row") | ||
| }) | ||
| } | ||
|
|
||
| // botCSVServer answers the org id, all-mannequins listing, REST bot lookup, user | ||
| // lookup, and both the reattributeMannequinToBot and createAttributionInvitation | ||
| // calls made by a mixed CSV reclaim (bot "example-ci[bot]" plus human "alice-t"). | ||
| // The returned bools are set when the bot mutation and the invitation are | ||
| // invoked, respectively. | ||
| func botCSVServer(t *testing.T) (srv *httptest.Server, botCalled, invited *bool) { | ||
| botCalled = new(bool) | ||
| invited = new(bool) | ||
| srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if strings.HasPrefix(r.URL.Path, "/users/") { | ||
| _, _ = io.WriteString(w, `{"type":"Bot","node_id":"BOT1"}`) | ||
| return | ||
| } | ||
| var req struct { | ||
| OperationName string `json:"operationName"` | ||
| } | ||
| require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) | ||
| switch req.OperationName { | ||
| case "GetOrganization": | ||
| _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) | ||
| case "ListMannequins": | ||
| _, _ = io.WriteString(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":"m1","login":"legacy-ci[bot]","claimant":null},{"id":"m2","login":"alice","claimant":null}]}}}}`) | ||
| case "GetUser": | ||
| _, _ = io.WriteString(w, `{"data":{"user":{"id":"u2"}}}`) | ||
| case "ReattributeMannequinToBot": | ||
| *botCalled = true | ||
| _, _ = io.WriteString(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":"m1","login":"legacy-ci[bot]"},"target":{"id":"BOT1","login":"example-ci[bot]"}}}}`) | ||
| case "CreateAttributionInvitation": | ||
| *invited = true | ||
| _, _ = io.WriteString(w, `{"data":{"createAttributionInvitation":{"source":{"id":"m2","login":"alice"},"target":{"id":"u2","login":"alice-t"}}}}`) | ||
| default: | ||
| require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName) | ||
| } | ||
| })) | ||
| return srv, botCalled, invited | ||
| } | ||
|
|
||
| // botClaimServer answers the org id, mannequins-by-login, REST bot lookup, and | ||
| // reattributeMannequinToBot calls made by a bot reclaim to target | ||
| // "example-ci[bot]". The returned bool is set to true when the bot mutation is | ||
| // invoked. | ||
| func botClaimServer(t *testing.T, mannequinLogin string) (*httptest.Server, *bool) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar thought here. Rather than having the handler inspect the query with For example: func botClaimServer(t *testing.T, mannequinLogin string) (*httptest.Server, *bool) {
const (
mannequinID = "m1"
botLogin = "example-ci[bot]"
botNodeID = "BOT1"
)
called := new(bool)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.HasPrefix(r.URL.Path, "/users/") {
_, _ = fmt.Fprintf(w, `{"type":"Bot","node_id":%q}`, botNodeID)
return
}
var req struct {
OperationName string `json:"operationName"`
}
require.NoError(t, json.NewDecoder(r.Body).Decode(&req))
w.Header().Set("Content-Type", "application/json")
switch req.OperationName {
case "GetOrganization":
_, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`)
case "ListMannequins":
_, _ = fmt.Fprintf(w,
`{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":%q,"login":%q,"claimant":null}]}}}}`,
mannequinID, mannequinLogin,
)
case "ReattributeMannequinToBot":
*called = true
_, _ = fmt.Fprintf(w,
`{"data":{"reattributeMannequinToBot":{"source":{"id":%q,"login":%q},"target":{"id":%q,"login":%q}}}}`,
mannequinID, mannequinLogin, botNodeID, botLogin,
)
default:
require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName)
}
}))
return srv, called
}I think this is preferable to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed this in dbea179 |
||
| const ( | ||
| mannequinID = "m1" | ||
| botLogin = "example-ci[bot]" | ||
| botNodeID = "BOT1" | ||
| ) | ||
| called := new(bool) | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if strings.HasPrefix(r.URL.Path, "/users/") { | ||
| _, _ = fmt.Fprintf(w, `{"type":"Bot","node_id":%q}`, botNodeID) | ||
| return | ||
| } | ||
| var req struct { | ||
| OperationName string `json:"operationName"` | ||
| } | ||
| require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) | ||
| switch req.OperationName { | ||
| case "GetOrganization": | ||
| _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) | ||
| case "ListMannequinsByLogin": | ||
| _, _ = fmt.Fprintf(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":%q,"login":%q,"claimant":null}]}}}}`, mannequinID, mannequinLogin) | ||
| case "ReattributeMannequinToBot": | ||
| *called = true | ||
| _, _ = fmt.Fprintf(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":%q,"login":%q},"target":{"id":%q,"login":%q}}}}`, mannequinID, mannequinLogin, botNodeID, botLogin) | ||
| default: | ||
| require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName) | ||
| } | ||
| })) | ||
| return srv, called | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could simplify this by dispatching GraphQL requests based on operationName rather than strings.Contains on the query. It would make the mock less brittle if the queries change and make it clearer which operations the test supports.
Something along these lines:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. Fixed this in dbea179