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

add Group kind #3909

Merged
merged 1 commit into from
Jul 28, 2015
Merged

add Group kind #3909

merged 1 commit into from
Jul 28, 2015

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Jul 24, 2015

Adds the Group kind and the groups resource.

@stevekuznetsov fyi

@deads2k deads2k force-pushed the groups branch 2 times, most recently from bf0e575 to efb80e7 Compare July 27, 2015 12:41
@deads2k
Copy link
Contributor Author

deads2k commented Jul 27, 2015

@Kargakis you're on the card, how about a review?

0xmichalis
0xmichalis reviewed Jul 27, 2015
View reviewed changes
pkg/client/groups.go
Get(name string) (*userapi.Group, error)
Create(group *userapi.Group) (*userapi.Group, error)
Update(group *userapi.Group) (*userapi.Group, error)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A method for deleting groups?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A method for deleting groups?

done

0xmichalis
0xmichalis reviewed Jul 27, 2015
View reviewed changes
pkg/user/api/validation/validation_test.go
@@ -7,6 +7,63 @@ import (
"github.com/openshift/origin/pkg/user/api"
)

func TestValidateGroup(t *testing.T) {
validObj := func() *api.Group {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test invalid names in groups too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test invalid names in groups too

done

@deads2k deads2k force-pushed the groups branch 2 times, most recently from ba13626 to ed670c6 Compare July 27, 2015 19:31
0xmichalis
0xmichalis reviewed Jul 27, 2015
View reviewed changes
pkg/api/deep_copy_generated.go
@@ -2303,6 +2303,52 @@ func deepCopy_api_TemplateList(in templateapi.TemplateList, out *templateapi.Tem
return nil
}

func deepCopy_api_Group(in userapi.Group, out *userapi.Group, c *conversion.Cloner) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these automatically generated? How?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are these automatically generated? How?

hack/update-generated-deep-copies.sh

@0xmichalis
Copy link
Contributor

Not sure what else needs tbd when adding a new resource. So far this code LGTM

@deads2k
Copy link
Contributor Author

deads2k commented Jul 27, 2015

Not sure what else needs tbd when adding a new resource. So far this code LGTM

  1. Scheme registration (no way to test this. intent isn't clear)
  2. API installer registration (no way to test this. intent isn't clear)
  3. create validator (if needed based on the storage, unit test will fail without this)
  4. update validator (if needed based on the storage, unit test will fail without this)
  5. printer (in merge queue, unit test will fail without this)
  6. describer (in merge queue, unit test will fail without this)
  7. update conversion, auto-complete, docs, deep_copy, swagger (verifies will fail without this)

A few standards (json tags, descriptions, more?) are enforced via unit tests as well. Basically, if you expose things to the point of being available, unit tests will start to smack you until you're in good shape.

@0xmichalis
Copy link
Contributor

Scheme registration (no way to test this. intent isn't clear)
API installer registration (no way to test this. intent isn't clear)
create validator (if needed based on the storage, unit test will fail without this)
update validator (if needed based on the storage, unit test will fail without this)
printer (in merge queue, unit test will fail without this)
describer (in merge queue, unit test will fail without this)
update conversion, auto-complete, docs, deep_copy, swagger (verifies will fail without this)

A few standards (json tags, descriptions, more?) are enforced via unit tests as well. Basically, if you expose things to the point of being available, unit tests will start to smack you until you're in good shape.

Thanks! This post should be documented somewhere.

@deads2k deads2k force-pushed the groups branch 2 times, most recently from 22e0852 to 4cb6740 Compare July 27, 2015 19:57
@deads2k
Copy link
Contributor Author

deads2k commented Jul 27, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2846/) (Image: devenv-fedora_2062)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to f809414

openshift-bot pushed a commit that referenced this pull request Jul 28, 2015
Merge pull request #3909 from deads2k/groups
3879f9c 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 3879f9c into openshift:master Jul 28, 2015
@deads2k deads2k deleted the groups branch July 31, 2015 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deads2k @0xmichalis @openshift-bot