-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[containerapp] az containerapp job start Ensure that args/cmd arguments are passed even without image argument
#32743
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1555,7 +1555,12 @@ def start_containerappsjob(cmd, | |
|
|
||
| template_def = None | ||
|
|
||
| if image is not None: | ||
| # Check if any container override parameters are provided | ||
| has_container_overrides = (image is not None or container_name is not None or | ||
| env_vars is not None or startup_command is not None or | ||
| args is not None or cpu is not None or memory is not None) | ||
|
|
||
| if has_container_overrides: | ||
| template_def = JobExecutionTemplateModel | ||
| container_def = ContainerModel | ||
| resources_def = None | ||
|
|
@@ -1565,7 +1570,25 @@ def start_containerappsjob(cmd, | |
| resources_def["memory"] = memory | ||
|
|
||
| container_def["name"] = container_name if container_name else name | ||
| container_def["image"] = image if not is_registry_msi_system(registry_identity) else HELLO_WORLD_IMAGE | ||
|
|
||
| # If no image is provided, fetch the existing job's image | ||
| if image is not None: | ||
| container_def["image"] = image if not is_registry_msi_system(registry_identity) else HELLO_WORLD_IMAGE | ||
| else: | ||
| # Fetch the existing job definition to get the default image | ||
| try: | ||
| containerappjob_def = ContainerAppsJobClient.show(cmd=cmd, resource_group_name=resource_group_name, name=name) | ||
| except Exception as e: | ||
| handle_raw_exception(e) | ||
|
|
||
| if not containerappjob_def: | ||
| raise ResourceNotFoundError("The containerapp job '{}' does not exist".format(name)) | ||
|
|
||
| existing_image = safe_get(containerappjob_def, "properties", "template", "containers", default=[{}])[0].get("image") | ||
| if not existing_image: | ||
| raise ValidationError("Could not find an existing image for the containerapp job '{}'. Please specify --image.".format(name)) | ||
| container_def["image"] = existing_image if not is_registry_msi_system(registry_identity) else HELLO_WORLD_IMAGE | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just noticed the original code looks weird: It set image to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These part of code are migrated from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Greedygre yeah this code really confused me. Looking more closely I'm seeing that this behavior is a bit different than container apps (although I think functionally it's the same). The default for the registry-identity is What are your thoughts? It seems reasonable to me to remove this logic about setting the default image to HelloWorld in the case that --registry-identity system` is passed (and to more closely align with container apps create). Thoughts? If you agree, I can perform this change in a subsequent PR (I think this would technically be breaking?) |
||
|
|
||
| if env_vars is not None: | ||
| container_def["env"] = parse_env_var_flags(env_vars) | ||
| if startup_command is not None: | ||
|
|
||
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 logic for fetching the existing job's image (lines 1578-1590) will be executed every time
has_container_overridesis true butimageis None. This means even if the user only wants to overridecpuormemorywithout changing the image, the code will unnecessarily fetch the job definition. Consider moving this fetch outside the conditional block or only fetching when needed for command/args overrides without an image.