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

WIP: Extract the uaa package to an external library #6

Merged

Conversation

joefitzgerald
Copy link
Contributor

Co-Authored-By: Caleb Washburn cwashburn@pivotal.io

Co-Authored-By: Caleb Washburn <cwashburn@pivotal.io>
@cfdreddbot
Copy link

Hey joefitzgerald!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/158203410

The labels on this github issue will be updated when the story is started.

@joefitzgerald joefitzgerald changed the title Extract the uaa package to an external library WIP: Extract the uaa package to an external library Jun 7, 2018
Copy link
Contributor

@DennisDenuto DennisDenuto left a comment

Choose a reason for hiding this comment

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

@joefitzgerald This PR looks great! nice work! We have some concerns about the changes made to listing groups, client and users.

@@ -13,8 +13,8 @@ func ListGroupValidations(cfg uaa.Config) error {
return nil
}

func ListGroupsCmd(gm uaa.GroupManager, printer cli.Printer, filter, sortBy, sortOrder, attributes string, startIndex, count int) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we have removed the ability to paginate the client/ groups / users endpoint. While I think this is convenient, we have concerns about performance as we know of some customers that have ~300,000 users.

We think that a nice compromise would be to keep this logic, however perhaps add the ability to 'opt-in' to listing all the users/groups/clients. (Perhaps a flag named --all). However, if this flag is not provided, then fallback to using pagination. Can we also add back startIndex and count?

@@ -14,8 +14,8 @@ func ListUserValidations(cfg uaa.Config) error {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

As per our previous comment, can we please add back pagination?

@joefitzgerald
Copy link
Contributor Author

Hey @DennisDenuto sure thing. This PR is based on v0.0.6 of the go-uaa dependency.

In v0.0.7 and v0.0.8 we add back functions that allow for listing a single page and listing all pages: https://github.com/cloudfoundry-community/go-uaa/releases

I can update the PR to make use of the latest release, but note thy the changeset will be significantly larger due to the breaking changes introduced in v0.0.7 and v0.0.8. Alternatively we could address that in a subsequent PR? What is your preference?

@DennisDenuto
Copy link
Contributor

@joefitzgerald Thanks for sharing that context. in that case I think it makes sense to merge with v0.0.6 swiftly followed by an upgrade to v0.0.8. The lack of pagination in between should not affect anyone since we have not yet released uaa-cli.

@drnic
Copy link

drnic commented Jun 28, 2018

After merging, I am getting failing test cases and my local files are being modified when running make && make install.

$ make && make install
...
Summarizing 4 Failures:

[Fail] ListUsers [It] understands the --zone flag
/Users/drnic/Projects/gopath/src/github.com/cloudfoundry-incubator/uaa-cli/vendor/github.com/onsi/gomega/ghttp/handlers.go:45

[Fail] ListGroups [It] understands the --zone flag
/Users/drnic/Projects/gopath/src/github.com/cloudfoundry-incubator/uaa-cli/vendor/github.com/onsi/gomega/ghttp/handlers.go:45

[Fail] ListUsers [It] executes SCIM queries based on flags
/Users/drnic/Projects/gopath/src/github.com/cloudfoundry-incubator/uaa-cli/vendor/github.com/onsi/gomega/ghttp/handlers.go:45

[Fail] ListGroups [It] executes SCIM queries based on flags
/Users/drnic/Projects/gopath/src/github.com/cloudfoundry-incubator/uaa-cli/vendor/github.com/onsi/gomega/ghttp/handlers.go:45

Ran 188 of 188 Specs in 6.365 seconds
FAIL! -- 184 Passed | 4 Failed | 0 Pending | 0 Skipped --- FAIL: TestCmd (6.37s)
FAIL
$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   Gopkg.lock
	modified:   cmd/add_member.go
	modified:   cmd/create_user.go
	modified:   cmd/deactivate_user.go
	modified:   cmd/errors.go
	modified:   cmd/get_password_token.go
	modified:   cmd/get_user.go
	modified:   cmd/refresh_token.go
	modified:   cmd/root.go
	modified:   cmd/set_client_secret.go
	modified:   cmd/target.go

@drnic
Copy link

drnic commented Jun 28, 2018

Perhaps can we cut releases of the old stable code before we resume merging + working on this repo again after a long break?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants