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.

Sign up for GitHub

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

Jump to bottom

Network v2: RBAC-LIST #758

Merged
merged 11 commits into from
Feb 22, 2018
Merged

Network v2: RBAC-LIST #758

merged 11 commits into from
Feb 22, 2018

Conversation

manju754
Copy link
Contributor

@manju754 manju754 commented Feb 13, 2018 •
edited

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 13, 2018

Build succeeded.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2018

Build failed.

@coveralls
Copy link

coveralls commented Feb 14, 2018 •
edited

Coverage Status

Coverage increased (+0.05%) to 73.506% when pulling 3f136ce on manju754:RBAC-GET-ALL into 94986f2 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2018

Build succeeded.

@manju754 manju754 changed the title [wip] Network v2: RBAC-LIST Network v2: RBAC-LIST Feb 14, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 14, 2018

Build succeeded.

@manju754
Copy link
Contributor Author

@jtopjian @kvrshenoy2 Please review this PR.

@jtopjian
Copy link
Contributor

@manju754 Thank you for your work on this. This PR should be rebased with master so that the non-rbac commits are removed. It might also make sense to wait until #755 is merged so those changes can be incorporated here.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 16, 2018

Build succeeded.

kvrshenoy2
kvrshenoy2 approved these changes Feb 16, 2018
View reviewed changes
Copy link

@kvrshenoy2 kvrshenoy2 left a comment

Choose a reason for hiding this comment

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

Functionality looks fine. Please follow the review comments on PR 755 on naming conventions.

@PriyankaJ77
Addressed doc nits, RBAC to RBACPolicy naming convention, and restric…
2545c85 
…ting the acceptance test to only be run by admin user.
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2018

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 20, 2018

Build succeeded.

@manju754
Copy link
Contributor Author

@jtopjian Can you review this PR. I had re-based the PR with master and resolved all the conflicts.

@jtopjian
Copy link
Contributor

@manju754 Thanks for continuing to work on the RBAC resources.

This will need rebased with master as there are some merge conflicts from the previous commit.

I have some questions with regard to some of the ListOpts fields:

  1. Why specify an ID? Isn't that the same as doing a Get?
  2. Why specify both ProjectID and TenantID? If they are both the same, let's standardize on ProjectID.

Also, it might be a good idea to add a unit test for AllPages. There have been a few times in the past where EachPage works but AllPages triggered an error. I think all of those bugs have been worked out, but it's still useful to test AllPages just in case.

Other than that, this looks good.

For posterity / others who read this PR: it isn't really possible to reference a single place in the Neutron code which defines valid List fields. Neutron has a set of standard fields (marker, limit, etc). The other fields are the fields/attributes of the resource itself.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 21, 2018

Build failed.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 21, 2018

Build succeeded.

@manju754
Copy link
Contributor Author

@jtopjian Can you review this PR. I had re-based the PR with master, resolved all the conflicts and addressed review comments.

@jtopjian
Copy link
Contributor

@manju754 All looks good but unfortunately it'll need another rebase from #781. Perhaps you and @PriyankaJ77 can figure out which PRs should be merged in order? I'm just going with what reaches my inbox first 😄

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 22, 2018

Build succeeded.

jtopjian
jtopjian requested changes Feb 22, 2018
View reviewed changes
Copy link
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@manju754 This looks good, but unfortunately the godoc formatting is off. I hate to request one more change, but the docs won't render correctly as they are.

Once that's fixed, this is good to go.

openstack/networking/v2/extensions/rbacpolicies/doc.go Outdated
if err != nil{
panic(err)
}
fmt.Printf("%+v", rbacpolicy)
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting will need fixed here - the code should be indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 22, 2018

Build succeeded.

@manju754
Copy link
Contributor Author

@jtopjian Can you review this PR. I had re-based the PR with master, resolved all the conflicts and addressed review comments.

@jtopjian
Copy link
Contributor

@manju754 Thanks so much for your patience with this. This looks good to go :)

@jtopjian jtopjian merged commit 8f1afb1 into gophercloud:master Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtopjian jtopjian jtopjian approved these changes

@kvrshenoy2 kvrshenoy2 kvrshenoy2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@manju754 @coveralls @jtopjian @kvrshenoy2 @PriyankaJ77