-
Notifications
You must be signed in to change notification settings - Fork 531
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
[Provisioner] Accept both w/ server prefix and w/o server prefix for private docker registry #2574
Conversation
server_prefix = f'{docker_login_config.server}/' | ||
if not specific_image.startswith(server_prefix): | ||
specific_image = f'{server_prefix}{specific_image}' |
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.
Should we handle this here or the _with_docker_login_config
in resources?
Pro for latter: we can assume the information provided to the node_provider or new provisioner always correct.
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.
Done. Thanks!
a840626
to
ae32801
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.
Thanks for the update @cblmemo! LGTM with a question.
logger.warning(f'{colorama.Fore.YELLOW}Docker login configs ' | ||
f'{", ".join(constants.DOCKER_LOGIN_ENV_VARS)} ' | ||
'are provided, but no docker image is specified ' | ||
'in `image_id`. The login configs will be ' | ||
f'ignored.{colorama.Style.RESET_ALL}') | ||
return resources | ||
return resources.copy(_docker_login_config=docker_login_config) | ||
# Already checked in extract_docker_image |
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.
Could you remind me why we do not allow multiple regions in image_id
when docker is specified?
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.
TBH I don't remember why... And I just finished a quick review, it seems possible to add this feature, though arguably this feature is a little bit unusual. IIUC the main reason we support multiple regions is image on AWS is not cross region?
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.
The mix of image and docker image seems possible too - just using the corresponding image when we make_deploy_resources_variables
will work iiuc. Let me file an issue to track on this
Co-authored-by: Zhanghao Wu <[email protected]>
Co-authored-by: Zhanghao Wu <[email protected]>
Tested (run the relevant ones):
bash format.sh
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh