-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
docker: add registry option #754
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #754 +/- ##
==========================================
- Coverage 18.99% 18.61% -0.38%
==========================================
Files 26 26
Lines 2290 2326 +36
==========================================
- Hits 435 433 -2
- Misses 1855 1893 +38
|
982a793
to
0306064
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just left a couple of cosmetic comments!
reana/reana_dev/docker.py
Outdated
@docker_commands.command(name="docker-push") | ||
def docker_push(user, tag, component): # noqa: D301 | ||
def docker_push(user, tag, component, registry, image_name): # noqa: D301 | ||
"""Push REANA component images to DockerHub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: this docstring should be amended to specify that DockerHub is not the only registry to which the image can be pushed, and to include the registry
and image_name
parameters.
if image_name and len(components) > 1: | ||
click.secho("Cannot use custom image name with multiple components.", fg="red") | ||
sys.exit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: maybe we could add this check also in the docker_push
function? It could be helpful when calling it directly from the docker-push
command.
reana/reana_dev/release.py
Outdated
@release_commands.command(name="release-docker") | ||
@click.pass_context | ||
def release_docker(ctx, component, user, image_name): # noqa: D301 | ||
def release_docker(ctx, component, user, image_name, registry): # noqa: D301 | ||
"""Release a component on Docker Hub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: same comment as above for the docstring
b565717
to
b546898
Compare
750bbe9
to
7258b98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely on Linux and macOS with Podman. Left some cosmetic comments.
reana/reana_dev/release.py
Outdated
@click.option( | ||
"--platform", | ||
multiple=True, | ||
help="Platforms for multi-arch images", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to specify [default=linux/amd64]
i.e. the default value for the platform. If it can be detected easily, we could print the actual value. Otherwise we can simply print "[current architecture]".
Also, it would be nice to modify top-level help to add the release example i.e. --platform linux/amd64 --platform linux/arm64
. Currently we have there the following:
$ reana-dev --help
...
How to release and push cluster component images:
.. code-block:: console
$ reana-dev git-clean
$ reana-dev docker-build --no-cache
$ # we should now run one more test with non-cached ``latest``
$ # once it works, we can tag and push
$ reana-dev docker-build -t 0.3.0.dev20180625
$ reana-dev docker-push -t 0.3.0.dev20180625
$ # we should now publish stable helm charts for tag via chartpress
reana/reana_dev/release.py
Outdated
@@ -84,10 +85,21 @@ def release_commands(): | |||
) | |||
@click.option("--user", "-u", default="reanahub", help="DockerHub user name [reanahub]") | |||
@click.option("--image-name", help="Should the component have a custom image name?") | |||
@click.option( | |||
"--registry", "-r", default="docker.io", help="Registry to use in the image tag" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to specify in the help string [default=docker.io]
explicitly, as we do for [reanahub]
the user name.
reana/reana_dev/release.py
Outdated
@@ -105,21 +117,85 @@ def release_docker(ctx, component, user, image_name): # noqa: D301 | |||
to include several runable REANA demo examples; | |||
* (7) special value 'ALL' that will expand to include | |||
all REANA repositories. | |||
:param user: Organisation or user name. [default=reanahub] | |||
:param image_name: Custom name of the local Docker image. | |||
:param registry: Registry to use in the image tag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add to the docstring [default=docker.io]
7258b98
to
b563170
Compare
Remove unused code that depends on the `VIRTUAL_ENV` environment variable.
Add option to only print the tags of Docker images to be released.
b563170
to
fc70577
Compare
Closes #752