[CLI] feat: add image selection and fix tests #5024
Conversation
|
|
||
|
|
||
| def pull_image() -> bool: | ||
| def pull_image(image: str = 'internal') -> bool: |
There was a problem hiding this comment.
Here internal is more a project then an image. You could rename the function to pull_image_for_project, and keep the behavior or actually receive the image here instead of the project name, and extracting the step of doing DOCKER_IMAGES[image] out of this function.
There was a problem hiding this comment.
You're right. I've renamed the project parameter and the pull_image_for_project function as you suggested. The concepts should be used correctly now.
|
You can address this in a separate PR, but it would be nice to rename the test files to follow the codebase pattern: |
+1 |
cli/casp/src/casp/commands/init.py
Outdated
| type=click.Choice( | ||
| docker_utils.PROJECT_TO_IMAGE.keys(), case_sensitive=False), | ||
| multiple=True) | ||
| def cli(projects: Tuple[str, ...]): |
There was a problem hiding this comment.
For this kind of typing, prefer using either abstract container types (like Sequence) or built-in type like tuple itself over the type alias. (go/pystyle#typing-imports)
| """Pulls the docker image.""" | ||
| click.echo(f'Pulling Docker image: {docker_utils.DOCKER_IMAGE}...') | ||
| if not docker_utils.pull_image(): | ||
| def _pull_image_for_project(project: str = 'internal'): |
There was a problem hiding this comment.
IMO this method should not have a default value for project. It should always expect it as an arg.
There was a problem hiding this comment.
Addressing in future PR.
| @click.command(name='init', help='Initializes the CLI') | ||
| def cli(): | ||
| @click.option( | ||
| '--projects', |
There was a problem hiding this comment.
Suggestion: Maybe the name project is not very clear for external users. I think config could be a bit more clear, wdyt?
There was a problem hiding this comment.
Addressing in future PR.
Context
This PR introduces a way to choose between
internal,externalanddevimmutable images when choosing the project in theinitcommand of the CLI and update the tests accordingly.For doing that, the old
DOCKER_IMAGEconstant is now a dictionary that maps the name of the project to the gcr path of the correspondent image. It was also added the option--project(or-p) to specify the project that the user wants to use and pull the correspondent image in the initialization. The user can select multiple projects, if he wants, ex.:Further work
The gcr path to the images are still hardcoded, this can be improved in future work.