Skip to content

Comments

Add integration test for cross-mounting blobs#25

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
dmage:integration-tests
Dec 8, 2017
Merged

Add integration test for cross-mounting blobs#25
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
dmage:integration-tests

Conversation

@dmage
Copy link
Contributor

@dmage dmage commented Dec 7, 2017

/assign @legionus

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 7, 2017
Copy link
Contributor

@legionus legionus left a comment

Choose a reason for hiding this comment

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

LGTM

// dead TCP connections (e.g. closing laptop mid-download) eventually
// go away.
type tcpKeepAliveListener struct {
*net.TCPListener
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it makes sense to give it a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is an embedded field on purpose.

@legionus
Copy link
Contributor

legionus commented Dec 7, 2017

Hm ...

if s := os.Getenv("REGISTRY_HTTP_TLS_MINVERSION"); len(s) > 0 {
if s := os.Getenv("REGISTRY_HTTP_TLS_CIPHERSUITES"); len(s) > 0 {

There are no such fields in the configuration [1][2]. It's unclear why it was necessary to name these environment variables like that.

I think it will be better to move them to our section in the configuration file. I don't mean in this PR.

[1] https://docs.docker.com/registry/configuration/#http
[2] https://github.com/openshift/image-registry/blob/master/vendor/github.com/docker/distribution/configuration/configuration.go#L91

@dmage
Copy link
Contributor Author

dmage commented Dec 8, 2017

/cc @miminar

@dmage dmage changed the title Add integration test for cross-mounting blobs [WIP] Add integration test for cross-mounting blobs Dec 8, 2017
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2017
@dmage dmage force-pushed the integration-tests branch from 9fd0f83 to 75a2c09 Compare December 8, 2017 17:33
@dmage dmage changed the title [WIP] Add integration test for cross-mounting blobs Add integration test for cross-mounting blobs Dec 8, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2017
@legionus
Copy link
Contributor

legionus commented Dec 8, 2017

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Details 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

@openshift-merge-robot openshift-merge-robot merged commit 85c2542 into openshift:master Dec 8, 2017
@dmage dmage deleted the integration-tests branch January 29, 2018 14:28
bverschueren added a commit to bverschueren/image-registry that referenced this pull request Sep 14, 2021
includes:
  - Support authentication using gcp workload identity federation (PR#26)
  - add application-credential auth support for swift storage driver (openshift#25)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants