-
Notifications
You must be signed in to change notification settings - Fork 564
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
NET-260 REST API tests #2351
NET-260 REST API tests #2351
Conversation
- added `/api/enrollment_keys` for non-admins - unit test for enrollment keys for non-admins
# Conflicts: # controllers/server.go
- return hosts with partial access
…st-api-tests # Conflicts: # controllers/enrollmentkeys.go
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.
fix order of imports in database/sqlite.go
database/sqlite.go
Outdated
@@ -3,14 +3,23 @@ package database | |||
import ( | |||
"database/sql" | |||
"errors" | |||
_ "github.com/mattn/go-sqlite3" // need to blank import this package |
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.
did you run go fmt and/or go import. stdlib imports should be listed first followed by external lib imports
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.
Fixed.
goimports
should be added to precommit. gofmt
orders it differently.
- goimports
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.
kitemake ticket is listed in on hold column so I didn't move it, but pr steps pass and critical list passes.
@theguy951357 It's not a user facing feature / release-bound code, so it doesnt have to be QA-ed. Thanks anyway! |
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.
assertion for last test?
} | ||
err = logic.CreateUser(user) | ||
if err != nil { | ||
t.Error("Error creating a user ", err) |
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.
could use t.Fatal
here?
adminConfigBad.MasterKey = "wrongpass" | ||
adminConfigBad.Password = "wrongpass" | ||
// add configs | ||
config.SetContext("user-ctx-1", userConfig) |
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.
the context names could be extracted as variables/consts maybe, to avoid writing it wrong and facing a difficult to debug situation
keys := *functions.GetEnrollmentKeys() | ||
assert.Len(t, keys, 1, "1 key expected") | ||
assert.Len(t, keys[0].Networks, 1, "Key with 1 network expected") | ||
assert.Equal(t, keys[0].Networks[0], n1.NetID, "Network ID matches") |
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.
are these messages not displayed on failure? honestly thought that but am not sure
// TODO doesnt return err | ||
res := *functions.GetEnrollmentKeys() | ||
fmt.Println(res) | ||
//assert.Error(t, res, "403 error") |
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.
should probably have an assertion here, len 0?
Describe your changes
NET-260 REST API tests for
GET /api/v1/enrollment-keys
.Provide testing steps
go test ./... -v -tags integration -test.run TestHasNetworksAccessAPI
Checklist before requesting a review