Skip to content
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

feat(cssc): 31231619 Enhance update command to check for Task yaml changes #13

Open
wants to merge 5 commits into
base: feature/cssc_ext
Choose a base branch
from

Conversation

cegraybl
Copy link

@cegraybl cegraybl commented Feb 5, 2025

This is a proposal to improve the experience when updating to a new version of the AZCLI extension.
This change modifies the behavior of the update command to silently check and update the Task definitions if there are any differences from the yamls defined on the extension.

This allows for users to move between versions and ensure that the task definition is always on the latest version, without requiring the user to delete and recreate the workflow, or for the hand holding to update the task definition on the customer's registry.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/acrcssc/azext_acrcssc/helper/_deployment.py:118

  • The word 'ust' should be corrected to 'us'.
# we need to know if tagging is something that will help ust,
@cegraybl cegraybl force-pushed the cegraybl/cssc_31231619_update_task_yaml branch from 460a399 to 2919af7 Compare February 14, 2025 23:30
@cegraybl cegraybl marked this pull request as ready for review February 14, 2025 23:32
@cegraybl cegraybl requested a review from Copilot February 14, 2025 23:32

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

src/acrcssc/azext_acrcssc/helper/_taskoperations.py:64

  • The task_list parameter could potentially lead to unexpected behavior if reused across multiple calls. Consider ensuring that task_list is either reset or checked for duplication before appending tasks.
cssc_tasks_exists = check_continuous_task_exists(cmd, registry, task_list=task_list)

src/acrcssc/azext_acrcssc/_validators.py:82

  • Ensure that the new behavior of check_continuous_task_exists with task_list is covered by tests.
def check_continuous_task_exists(cmd, registry, task_list=None):

src/acrcssc/azext_acrcssc/helper/_deployment.py:118

  • The word 'ust' is misspelled. It should be 'us'.
# we need to know if tagging is something that will help ust,
@cegraybl cegraybl changed the title feat(cssc): 31231619 Enhance update command with '--force-task-update' to allow for task yaml updates feat(cssc): 31231619 Enhance update command to check for Task yaml changes Feb 14, 2025
continue
deployed_task = task.step.encoded_task_content
extension_task = _create_encoded_task(CONTINUOUSPATCH_TASK_DEFINITION[task.name]["template_file"])
if deployed_task != extension_task:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than checking the content, shall we also check if schedule_cron_expression is null? Either change should trigger a task update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the function gets schedule_cron_expression as None, it will retrieve the cron from the trigger task.
Do you mean that we should check if the returned task also has a null cron expression (i.e. the customer modified the task to remove the schedule?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There're two scenarios where we should update the task, either when the task content (yaml) changes or schedul_cron_expression schedule is modified, right? If schedul_cron_expression is non, it means "No update", else it might means "update". So when to decide if to update the task, it should be based on 1. whether deployed_task is equal to extension_task 2. Whether schedule_cron_expression is null, e.g.

if deployed_task != extension_task or schedule_cron_expression is None:
...

In your current code, you only check if the task is equal or not. If it is not equal, you then check the schedule_cron_expression.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, the case when the deployed task and the extension task are the same and the schedule needs to be updated is on line 137 under _update_task_schedule.

This section is meant to handle only when the tasks are different, and if we don't have a cron expression (only the configuration is being updated) we need to retrieve it from the task to keep it the same, as the ARM template includes it as as parameter.

@cegraybl cegraybl requested a review from Copilot February 20, 2025 23:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

add checks for the trigger task timer before attempting to update

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@cegraybl cegraybl requested a review from huanwu February 24, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants