Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Add ability to view and manage roles for spaces and orgs. #48

Closed
wants to merge 2 commits into from

Conversation

jmprice
Copy link

@jmprice jmprice commented Aug 21, 2013

No description provided.

new module to allow management of space roles
New module for management of organization roles
@cfdreddbot
Copy link

Hey jmprice!

Thanks for submitting this pull request!

All pull request authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

@jbayer
Copy link

jbayer commented Aug 21, 2013

@jmprice is part of an organization that has signed the corporate CLA. This contribution can go forward from that perspective.

@ryantang
Copy link
Contributor

@jrichard - would you mind taking a look at this and seeing if this is a feature that we'd like to support in cf?

@jfoley / @ryantang

@jbayer
Copy link

jbayer commented Aug 25, 2013

@jmprice conceptually, i agree that role management actions should be in the cli.

not having tried the interactions myself, they appear to be in the right spirit of things looking at the source. but i would like to try them.

however, these commits do not have associated tests from what i saw, and i suspect the eng team will be hesitant to accept it as-is. i have asked the team working on the cli to look into this.

@dsabeti
Copy link

dsabeti commented Aug 29, 2013

@jmprice

I've reviewed the pull request and think it needs some work. Foremost, the commit needs tests to verify that the functionality is good. Take a look at the file called cli_helper.rb -- it provides some functions for simulating the command line experience and capture/asserting on output.

Some notes on code:

  1. The 'argument' parameter for the action input should perhaps be set to true. This means that the command requires that argument be specified at the command line. The user would have to type cf roles list or cf roles add rather than allow the user to be prompted for the action later on. It's a ux decision, but it's more in line with our other commands where the verb is required at the command line.
  2. You get the --space flag for free when you define the input. No need to alias it. Same with --role, --action, and --email.
  3. There are a few blocks in the code that look like this:
if quiet?
    puts space.name
    return
end

I'm not sure I understand the purpose of this block. What's the intention of the --quiet flag and why should the program exit here?

  1. The get_user_guid method may need some refactoring. Mostly in that the return values can be either a CFoundry::V2::User, a String, or nil, and also in that it can invoke org_roles as a side-effect. The method should probably only return a User object or nil (never a string), and the additional invoke should be put into a different part of the code.

When you've added the specs, rebase and re-submit the pull request.

@dsabeti dsabeti closed this Aug 29, 2013
@jmprice
Copy link
Author

jmprice commented Aug 29, 2013

Thank you for reviewing the code. I am not a ruby developer by any stretch of the imagination but was frustrated by the lack of tools for managing roles in orgs and spaces so I created something that works for me. I was hoping you would find it useful and incorporate the idea into the cf gem itself.

@mkocher
Copy link

mkocher commented Aug 29, 2013

I think this may have been closed in error. @jmprice, are you interested in following up on @dsabeti's feedback? We should leave this open if you're going to work on it.

@dsabeti
Copy link

dsabeti commented Aug 29, 2013

Hehe, good catch Matthew. Clicked the wrong button. Reopening...

@dsabeti dsabeti reopened this Aug 29, 2013
@jbayer
Copy link

jbayer commented Aug 30, 2013

@jmprice just in case you do not have the time/interest in getting the role management code to adjust to some of the engineering feedback, you have my commitment that we'll put role management in our backlog of cli features.

@jmprice
Copy link
Author

jmprice commented Aug 30, 2013

Hey James, thank you. I will definitely try to refine my code but it will probably be a while until I'm able to get back to it, other work has me pretty swamped for the immediate future.

@jmprice jmprice closed this Aug 30, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants