support private quay instance #171
Conversation
openshift-ci-robot
added
the
size/S
operatorcourier/push.py
Outdated
| """Push takes a bundle and pushes it to the specified app registry repository. | ||
|
|
||
| :param bundle_dir: Path to generated local directory that contains the bundle. | ||
| :param namespace: Namespace that contains the repository for the application. | ||
| :param repository: Repository name of the application described by the bundle. | ||
| :param release: Release version of the bundle. | ||
| :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. |
There was a problem hiding this comment.
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``.
There was a problem hiding this comment.
@MartinBasti Thanks! I have updated the documentation.
There was a problem hiding this comment.
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?
| :param verify_host: 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``. | ||
| """ | ||
| logger.info('Generating 64 bit bundle and pushing to app registry.') | ||
| filterOutFiles(bundle_dir, BLACK_LIST) |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
fixes #170