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

Return role info for each role on pathRoleList #3532

Merged
merged 4 commits into from
Nov 3, 2017
Merged

Conversation

calvn
Copy link
Contributor

@calvn calvn commented Nov 3, 2017

No description provided.

@calvn calvn added this to the 0.8.4 milestone Nov 3, 2017
// with appropriate values for each keyType. If the keyType is invalid, it will retun
// an error.
func (b *backend) parseRole(role *sshRole) (map[string]interface{}, error) {
result := map[string]interface{}{}
Copy link
Member

Choose a reason for hiding this comment

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

Is this intended to ensure that this method doesn't return a nil? If not, the implicit make here can be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea since we are assigning whole result map in the switch statement, we don't have to initialize it right away.

vishalnayak
vishalnayak previously approved these changes Nov 3, 2017
Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

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

One minor comment. LGTM! (after the s/roles/key_info)


// ListResponseWithInfo is used to format a response to a list operation and
// return the keys as well as a map with corresponding key info.
func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Response {
Copy link
Member

Choose a reason for hiding this comment

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

Ooh interesting.

func ListResponseWithInfo(keys []string, keyInfo map[string]interface{}) *Response {
resp := ListResponse(keys)

resp.Data["key_info"] = map[string]interface{}{}
Copy link
Member

Choose a reason for hiding this comment

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

You should create the map separately and assign to resp.Data at the end of the response. This avoids having to keep type asserting in the loop...cleaner and faster!

Separately, I'm not sure whether, if there are no actual keys, we should return an empty map, a null value, or not add the key at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with not returning "key_info" at all if it's empty, similar to the behavior with "keys".

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

jefferai
jefferai previously approved these changes Nov 3, 2017
@calvn calvn merged commit ca76bc4 into master Nov 3, 2017
@calvn calvn deleted the ssh-list-roles-info branch November 3, 2017 21:12
chrishoffman pushed a commit that referenced this pull request Nov 7, 2017
* oss/master: (30 commits)
  Handle 'not supplied' case for field type TypeNameString (#3546)
  Fix deprecated cassandra backend tests (#3543)
  changelog++
  auth/aws: Make disallow_reauthentication and allow_instance_migration mutually exclusive (#3291)
  changelog++
  More Mount Conflict Detection (#2919)
  Fix swallowed errors in TestRollbackManager_Join() (#3327)
  changelog++
  added AWS enpoint handling (#3416)
  Seal wrap all root tokens and their leases (#3540)
  Return group memberships of entity during read (#3526)
  Add note on support for using rec keys on /sys/rekey (#3517)
  Add third party tools list to website (#3488)
  Minor client refactoring (#3539)
  changelog++
  Add PKCS8 marshaling to PKI (#3518)
  Update SSH list roles docs (#3536)
  Update gocql dep
  changelog++
  Return role info for each role on pathRoleList (#3532)
  ...
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.

3 participants