Skip to content

Conversation

@jkandasa
Copy link

fixes #170

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 22, 2019
:param auth_token: Authentication token used to push to Quay.io.
:param quay_host: Can be 'quay.io' or your private instance hostname.
:param verify_host: ``quay_host`` TLS/CA verification.
Either a boolean or a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put here a full documentation, or refer to requests documentation what usage of string means here.
From requests

    :param verify: (optional) Either a boolean, in which case it controls whether we verify
            the server's TLS certificate, or a string, in which case it must be a path
            to a CA bundle to use. Defaults to ``True``.

Copy link
Author

Choose a reason for hiding this comment

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

@MartinBasti Thanks! I have updated the documentation.

@jkandasa jkandasa requested a review from MartinBasti October 22, 2019 10:51
Copy link
Member

@kevinrizza kevinrizza left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! It looks good. I just have a few minor things:

If we are going to make the host path configurable, we should probably make it non specific so that any arbitrary implementation of the app-registry protocol is supported.

Can you also update the CLI package to include these new parameters? I would prefer that those did not diverge.

Can you update README.MD to reference these changes?

Defaults to ``True``.
"""
logger.info('Generating 64 bit bundle and pushing to app registry.')
filterOutFiles(bundle_dir, BLACK_LIST)
Copy link
Member

Choose a reason for hiding this comment

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

Just below this, don't you also need to update the call to _push_to_registry() to actually pass the new parameters along?

push_uri = 'https://quay.io/cnr/api/v1/packages/%s/%s' % (namespace, repository)
def _push_to_registry(self, namespace, repository, release, bundle, auth_token,
quay_host, verify_host):
push_uri = 'https://%s/cnr/api/v1/packages/%s/%s' % (quay_host, namespace,
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to do this, rather than just support additional quay.io/cnr endpoints, why not just make the entire root path of the host to the API configurable? i.e.:

push_uri = 'https://%s/api/v1/packages/%s/%s' % (host, namespace,repository)

https://quay.io/cnr is the full arbitrary root path, the cnr is not part of the app-registry spec. Here's a full client implementation for reference: https://github.com/operator-framework/go-appr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for private quay instance

4 participants