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 dockerregistry/server.App struct for application-level data #15796

Merged
merged 1 commit into from
Sep 15, 2017
Merged

Add dockerregistry/server.App struct for application-level data #15796

merged 1 commit into from
Sep 15, 2017

Conversation

dmage
Copy link
Contributor

@dmage dmage commented Aug 16, 2017

No description provided.

@dmage dmage requested a review from legionus August 16, 2017 15:11
@openshift-merge-robot openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 16, 2017
@dmage dmage requested a review from miminar August 16, 2017 15:11
@dmage
Copy link
Contributor Author

dmage commented Aug 16, 2017

/unassign @liggitt
/unassign @bparees

@bparees
Copy link
Contributor

bparees commented Aug 16, 2017

/unassign @bparees

miminar
miminar suggested changes Aug 17, 2017
View reviewed changes
pkg/dockerregistry/server/app.go Outdated
registryClient RegistryClient
extraConfig *registryconfig.Configuration

// dockerStorageDriver gives access to the blob store.
Copy link

Choose a reason for hiding this comment

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

outdated name

pkg/dockerregistry/server/app.go Outdated
// cases it is hidden from us.
driver storagedriver.StorageDriver

// dockerRegistry represents a collection of repositories, addressable by name.
Copy link

Choose a reason for hiding this comment

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

same as before

pkg/dockerregistry/server/middlewares.go Outdated
)

const (
AuthOpenShift = "openshift"
Copy link

Choose a reason for hiding this comment

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

godoc

pkg/dockerregistry/server/middlewares.go Outdated
@@ -0,0 +1,88 @@
package server
Copy link

Choose a reason for hiding this comment

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

Can we name it register.go or middlewareinit.go or just init.go?

Can we also rename repositorymiddleware to repository?

pkg/dockerregistry/server/middlewares.go Outdated
return nil, fmt.Errorf("failed to find an application instance in the context of the registry middleware")
}

log.Info("OpenShift registry middleware initializing")
Copy link

Choose a reason for hiding this comment

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

Can we ditch the product name? E.g. replace the OpenShift with downstream or origin or just lowercased openshift to refer to a config section name instead of product.

pkg/dockerregistry/server/repositorymiddleware.go Outdated
@@ -222,6 +196,7 @@ func newRepositoryWithClient(
Repository: repo,

ctx: ctx,
app: app,
Copy link

Choose a reason for hiding this comment

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

Why making it part of this struct if it is already contained in the context. Accessing the whole app from inside of middleware shouldn't be that easy.

@miminar
Copy link

miminar commented Aug 17, 2017

👍 otherwise. I like 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 Aug 29, 2017
@openshift-merge-robot openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 30, 2017
@soltysh
Copy link
Member

soltysh commented Aug 30, 2017

/unassign
/assign @miminar

@openshift-merge-robot openshift-merge-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 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 8, 2017
@dmage
Copy link
Contributor Author

dmage commented Sep 8, 2017

/retest

@dmage
Copy link
Contributor Author

dmage commented Sep 8, 2017

flake #16248
/retest

@dmage
Copy link
Contributor Author

dmage commented Sep 11, 2017

@miminar I rebased this PR, PTAL again.

@miminar
Copy link

miminar commented Sep 11, 2017

/test extended_image_registry

miminar
miminar suggested changes Sep 11, 2017
View reviewed changes
Copy link

@miminar miminar left a comment

Choose a reason for hiding this comment

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

It's getting close..

pkg/dockerregistry/server/app.go Outdated
return nil, fmt.Errorf("configuration error: the storage driver middleware %q is not activated", middlewareOpenShift)
}

if app.registry == nil {
Copy link

Choose a reason for hiding this comment

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

Can these asserts be moved into NewApp()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, it checks that the middlewares are initialised in the proper order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

pkg/dockerregistry/server/app.go Outdated
return nil, err
}

return app.newRepositoryWithClient(ctx, registryOSClient, repo)
Copy link

Choose a reason for hiding this comment

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

Why do we need second method that just accepts client obtained by the first, which could be obtained by the second as well?

pkg/dockerregistry/server/app.go

// Registry extensions endpoint provides extra functionality to handle the image
// signatures.
RegisterSignatureHandler(dockerApp)
Copy link

Choose a reason for hiding this comment

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

nit/joke: I need to remember to register next special handler using locally defined closure or something more creative.

@miminar
Copy link

miminar commented Sep 13, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@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 14, 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 14, 2017
@legionus
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, legionus, miminar

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

@legionus
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 15, 2017 •
edited

@dmage: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_image_registry 6afe7b5 link /test extended_image_registry

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@legionus
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15725, 16244, 15796, 16328, 16334)

@openshift-merge-robot openshift-merge-robot merged commit 6af6cb2 into openshift:master Sep 15, 2017
@dmage dmage mentioned this pull request Sep 16, 2017
Closed
@mfojtik mfojtik mentioned this pull request Sep 17, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miminar miminar miminar approved these changes

@legionus legionus legionus approved these changes

Assignees

@miminar miminar

@legionus legionus

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@dmage @bparees @miminar @soltysh @stevekuznetsov @openshift-bot @legionus @openshift-merge-robot @openshift-ci-robot @liggitt