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

Switch pkg/image to use clientsets #16195

Merged
merged 3 commits into from Sep 19, 2017
Merged

Switch pkg/image to use clientsets #16195

merged 3 commits into from Sep 19, 2017

Conversation

soltysh
Copy link
Member

@soltysh soltysh commented Sep 7, 2017

@mfojtik @deads2k I hope this handles all of them, including the SAR bits we've talked about yesterday.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k
deads2k reviewed Sep 7, 2017
View reviewed changes
pkg/image/registry/imagestreamimport/rest.go Outdated
createImageSAR := authorizationutil.AddUserToSAR(user, &authorizationapi.SubjectAccessReview{
Spec: authorizationapi.SubjectAccessReviewSpec{
ResourceAttributes: &authorizationapi.ResourceAttributes{
Namespace: inputMeta.Namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this namespace scoped before? images are cluster scoped.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is imagestreamimport, not image. Iow. you create a resource that tells the server to import some data inside particular image stream.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take that back. Yes, you're right this should not be namespaced.

deads2k
deads2k reviewed Sep 7, 2017
View reviewed changes
pkg/image/registry/imagestreamimport/rest.go Outdated
createImageStreamMappingSAR := authorizationutil.AddUserToSAR(user, &authorizationapi.SubjectAccessReview{
Spec: authorizationapi.SubjectAccessReviewSpec{
ResourceAttributes: &authorizationapi.ResourceAttributes{
Namespace: inputMeta.Namespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

I expected this one to be namespace scoped, but it looks like it wasn't namespaced before. Was it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not, I'm removing the namespace parameter.

deads2k
deads2k reviewed Sep 7, 2017
View reviewed changes
pkg/image/apis/image/helper.go
@@ -355,6 +350,11 @@ func JoinImageStreamTag(name, tag string) string {
return fmt.Sprintf("%s:%s", name, tag)
}

// JoinImageStreamImage creates a name for image stream image object from an image stream name and an id.
func JoinImageStreamImage(name, id string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change? Make reads better to me for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to make it consistent with the other helper JoinImageStreamTag.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll split it to a separate commit.

deads2k
deads2k reviewed Sep 7, 2017
View reviewed changes
pkg/image/prune/prune.go
return &imageDeleter{
images: images,
}
}

func (p *imageDeleter) DeleteImage(image *imageapi.Image) error {
glog.V(4).Infof("Deleting image %q", image.Name)
return p.images.Delete(image.Name)
return p.images.Images().Delete(image.Name, metav1.NewDeleteOptions(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

old code was this aggresive with the delete options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm... there was no such option in the old code, and since this is pruning I assumed this will be the best option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... there was no such option in the old code, and since this is pruning I assumed this will be the best option.

pass nil or empty object. Pretty sure this one takes a nil.

@deads2k
Copy link
Contributor

deads2k commented Sep 7, 2017

errors look real.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2017
@deads2k
Copy link
Contributor

deads2k commented Sep 13, 2017

I think the questions are still outstanding.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2017
@soltysh
Copy link
Member Author

soltysh commented Sep 15, 2017

@deads2k comments addressed, problems solved and rebase. ptal

deads2k
deads2k reviewed Sep 15, 2017
View reviewed changes
pkg/image/apiserver/apiserver.go
if err != nil {
return nil, err
}
coreClient, err := coreclient.NewForConfig(c.CoreAPIServerClientConfig)
authorizationClient, err := authorizationclient.NewForConfig(c.GenericConfig.LoopbackClientConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

preexisting, but this one should probably be CoreAPIServerClientConfig

@deads2k
Copy link
Contributor

deads2k commented Sep 15, 2017

The delete options and the client config bit. Otherwise, lgtm. After that feel free to tag it.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2017
@soltysh
Copy link
Member Author

soltysh commented Sep 18, 2017

Rebased, applying the label based on approval.

@soltysh soltysh added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Sep 18, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2017
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 19, 2017
@soltysh
Copy link
Member Author

soltysh commented Sep 19, 2017

Rebased, re-applying the label back.

@soltysh soltysh added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2017
@soltysh
Copy link
Member Author

soltysh commented Sep 19, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit 16fabd4 into openshift:master Sep 19, 2017
@soltysh soltysh deleted the image_clientsets branch September 19, 2017 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@mfojtik mfojtik

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@soltysh @openshift-merge-robot @deads2k @mfojtik @openshift-ci-robot