ACR: Add Pull Request Event to Task Source Trigger #7526

jaysterp · 2018-10-09T23:41:44Z

ACR Task will now support both commit and pull request source trigger events types.

msftclas · 2018-10-09T23:41:56Z

All CLA requirements met.

djyou · 2018-10-19T18:33:42Z

src/command_modules/azure-cli-appservice/HISTORY.rst

@@ -2,6 +2,10 @@

 Release History
 ===============
+0.2.6
+++++
+* support commit and pull request git events for ACR Task source trigger


This is for app service. I'd keep it to something more general such as update ACR SDK

djyou · 2018-10-19T18:39:13Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py

-                source_trigger_events=[SourceTriggerEvent.commit.value],
-                status=TriggerStatus.enabled.value if commit_trigger_enabled else TriggerStatus.disabled.value,
+                source_trigger_events=source_trigger_events,
+                status=TriggerStatus.enabled.value if source_trigger_events else TriggerStatus.disabled.value,


Do you need to check if source_trigger_events again?

Also it seems redundant for the client to provide this status, can the server side determine and return the correct status based on the value of source_trigger_events? Or is it possible to have a case where source_trigger_events is not empty but status is disabled? Or source_trigger_events is empty but status is enabled?

Yes, for task create, the status will always be enabled as long as source_trigger_events is not empty. As soon as the event list is empty, status is set to disabled. However, in the near future we will support scenarios where triggers can be set to disabled even when source_trigger_events is not empty. In that case it is useful for the client to provide status.

See fix in most recent update.

djyou · 2018-10-19T18:43:03Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py

@@ -488,3 +497,27 @@ def _get_list_runs_message(base_message, task_name=None, image=None):
    if image:
        base_message = "{} for image '{}'".format(base_message, image)
    return "{}.".format(base_message)
+
+
+def _get_trigger_event_list(source_triggers,


nit: _update_trigger_event_list

The event list is not always updated. If a user calls task update for reasons other than changing the status of commit-trigger-enabled or pull-request-trigger-enabled then the event list is not updated, but retrieved as is from server.

Fair enough, I was under the impression that this one can also be called by the create method.

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py

djyou · 2018-10-19T18:52:35Z

src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py

+    if source_triggers:
+        source_trigger_events = set(source_triggers[0].source_trigger_events)
+        if source_triggers[0].status == TriggerStatus.disabled.value:
+            source_trigger_events.clear()


This also relates to the above question. Is it possible that source_trigger_events is not empty (so you are clearing it) but status is disabled?

There is an edge case in the RP that checks if source_trigger_events is empty. During update, if the user disabled all events in a trigger and sends an empty source_trigger_events to the RP, then the RP overrides these events with those of the existing trigger.

sourceTrigger.SourceTriggerEvents = sourceTrigger.SourceTriggerEvents?.Count() > 0 ? sourceTrigger.SourceTriggerEvents : existingTrigger.SourceTriggerEvents;

Therefore, a disabled trigger event list may be "empty" to the client it is not to the server (though the trigger is still disabled). This .clear() is here because if a user is calling update again on a trigger that has been disabled, then we want to make sure none of the previously disabled events events are added back without the user knowing.

djyou

LGTM

jaysterp · 2018-10-22T20:05:04Z

@yugangw-msft @tjprescott we are ready to merge. Can you please take a look? Thanks!

jaysterp · 2018-10-23T21:49:46Z

// cc @sajayantony

@shanepeckham

* Progress (Azure#7558) * update knack 0.4.4 (Azure#7567) * Update CODEOWNERS (Azure#7571) * Add table for exit codes (Azure#7572) * Updated adls version to 0.0.34 (Azure#7570) * Updated adls version to 0.0.33 * Updated version * Update version * Downgrade to 0.0.32 due to test failures * Updated test recordings with new version and updated to version 0.0.34 again * Updated History.rst * Merge CLI 2.0.48 hotfix to dev and bump versions (Azure#7590) * Homebrew hotfix. * CI issue? * make scrubbed over value valid base64 (Azure#7592) * Graph: support add/remove/list owners on app, sp, and group (Azure#7578) * Container Instance: Updating VNET workflow (Azure#7517) * Adding vnet-resource-group * fixing wording for more clear content * Removing vnet-resource-group * allow passing in vnet name or id * updating container module version * adding wording change for the LRP in container warning. * reverting version back to current version * Adding private option for --ip-address * updating tests for new parameters * updating tests recordings * Removing the ability to create VNET by passing the resource ID * adding --vnet-name back for backwards compatability * Fixing based on feedback * Bumping the version for CItests * updating container instance sdk version * changing casing and removing existance check * fixing no image error validation * Fix CI. Remove autopep8 dependency (Azure#7597) * Update tools setup. * Remove autopep8 * Iot Central Added Display Name and Template Parameters for App Create (Azure#7518) * [Storage] allow deletion of containers with immutability policy after service bug fix (Azure#7569) * delete container to custom command * finished up commands * added test * history and pep8 * linter, static * new recording after scrubbing fix * comment typo * vmss: avoid producing useless stroage accounts with unmanaegd scaleset (Azure#7601) * [Monitor] Fix `activity-log list` issues. (Azure#7525) * Fix Azure#5608. Fix Azure#4885. * Update Metric command. * Container Instance: Role Assignment for System Assigned MSI (Azure#7577) * adding deployment based for resources * changing to sdk calls for role assignment * Fixing naming problem causing authorization client to fail * Fixing formatting errors * fixing differnt return statemnet errors * adding tests for the --scope parameter * fixing conflicts in recordings * fixing conflicts and rerecording tests * removing whitespace * making non predictble test live only * Fixing blank lines * allow role assignment for --no-wait aswell * removed unused imports * CLI now prints extension messages. E.g. This extension is in preview. The behavior of this command has been altered by the following extension: storage-preview (Azure#7606) * Removing Warnings for Cloudshell Proxy (Azure#7605) * removing warnings for cloudshell proxy * updating code owners for the container modules * Updating how env var is accessed * fixing formatting error in string message * removing unused import * Bump versions for next CLI release (Azure#7610) * AAD Graph: support grant/list app permissions(Azure#7611) Contributed by: @shanepeckham Payload details: https://blogs.msdn.microsoft.com/arsen/2017/07/30/azure-ad-how-to-create-oauth2permissiongrant-using-graph-api-grant-permissions-and-consent-for-application/ * Fixes Azure#7596. (Azure#7620) * Retry webbrowser.open() after a TypeError (Azure#7542) * [Monitor] Metric alerts - allow special characters in metric name (Azure#7623) * Fixes Azure#7598. * Add test. * Closes Azure#7545. * moved interactive events to core (Azure#7632) * Fixed bug where --no-wait option causes vm resize to crash. (Azure#7627) * Fixed bug where --no-wait option causes vm resize to crash. * Bump up VM version number * Update rdbms help examples text from Gen4 to Gen5 (Azure#7642) * Update rdbms help examples text from Gen4 to Gen5 * Fix version * Fix public-ip create. (Azure#7638) * image create now accepts a storage-sku. (Azure#7614) * image create now accepts a storage-sku. Unused when source is a VM due to service side issue. Also updated help text and made some changes to some model imports. * Updated storage-sku help parameter * Fixed bug introduced by using get_models. For profiles that don't have StorageAccountTypes defined define the possible storage-sku values. * History changes. * Bumped up VM version number. removed extra newline in History.rst * Added test coverage for --storage-sku * Comment explaining else statement in vm/_params.py * Modified test_vm_managed_disk * Addressed pep8 warnings. * ACR: Add Pull Request Event to Task Source Trigger (Azure#7526) * initial work on the PR trigger * task create and update with PR trigger * use one flag to enable commit and pr trigger events * nit fixes * Two event flags and only support one source trigger per task * PR fixes * allow empty event list when disabling all trigger events * format fix * do not clear event list in update, instead status disabled * update task table * nit fix * bump cli event version * nit fixes * update ACR sdk version * version fix * update appservice version * pylint fixes * pylint fixes * nit * pylint fix * respond to PR comments * change from null to empty check * ad: clarify the confusion between displayName and service principal name (Azure#7651) * [setup]: windows installer improvement (Azure#7633) * revert to desktop python * change before do model file concat * add trimmer * revert an unrelated change * incorporate model file trimmer * revert a few unnecessary change * lint fix * remove an irrelevant command file * backup python to storage accounts * remove irrelevant comment * simplify * Closes Azure#7536 (Azure#7626) * pin flake8 to 3.5.0 (Azure#7663) * pin flake8 to 3.5.0 * wrong version * setup: add back the workaround of pinning pycparser==2.18 (Azure#7662) * Fixed bug with update --remove --ids (Azure#7643) * Fixed bug where if update --remove is called with --ids, the correct params aren't passed in subsequent iterations of the update command. * Fixed Pep8 issues * Fixed bug by changing generic update logic instead of copying CLI params. * Move extension logic into core (Azure#7653) * moved previous 'extensions' event handling logic * moved all extension management utility to core * fix ref static checking fix test import * core package * fix reference * renamed to register_global_transforms * Update to latest Advisor SDK (Azure#7576) * Updates for GA release. * Addressing PR feedback: - Removed 'recommendation generate' command and added a '--refresh' option to 'recommendation list' instead. - Renamed 'configuration get' to 'configuration list'. - Renamed 'configuration set' to 'configuration update'. - Test recording updates. * Update history with details of the changes. * Adding generic update and show command. * Addressing review feedback. * Update SDK package version. * Addressing review feedback. * Fix indentation errors. * Add support for ids. * Updates for GA release. * Addressing PR feedback: - Removed 'recommendation generate' command and added a '--refresh' option to 'recommendation list' instead. - Renamed 'configuration get' to 'configuration list'. - Renamed 'configuration set' to 'configuration update'. - Test recording updates. * Update history with details of the changes. * Adding generic update and show command. * Addressing review feedback. * Update SDK package version. * Addressing review feedback. * Fix indentation errors. * Add support for ids. * Fix test failures. * Update to latest SDK version * Update history * Reference new package version * Fixed non-working example for mysql db create (Azure#7557) * Update _help.py Fixed non-working example with guidance from https://docs.microsoft.com/en-us/azure/mysql/quickstart-create-mysql-server-database-using-azure-cli * Update HISTORY.rst * Update setup.py * Fix placeholder * Fix Azure#7659 (Azure#7668)

ankurkhemani approved these changes Oct 10, 2018

View reviewed changes

jaysterp requested a review from yugangw-msft October 18, 2018 18:50

jaysterp added 14 commits October 18, 2018 12:08

initial work on the PR trigger

1accdca

task create and update with PR trigger

20866b9

use one flag to enable commit and pr trigger events

6b87f94

nit fixes

bdfd98d

Two event flags and only support one source trigger per task

b0b3d46

PR fixes

94d0527

allow empty event list when disabling all trigger events

d2ddfb2

format fix

7ff22c2

do not clear event list in update, instead status disabled

1eb7626

update task table

4961413

nit fix

a998801

bump cli event version

d95e1ce

nit fixes

dbd7ced

update ACR sdk version

e40a2a5

jaysterp force-pushed the jeburke/pr-trigger branch from 76cf634 to e40a2a5 Compare October 18, 2018 19:16

jaysterp added 6 commits October 18, 2018 12:34

version fix

4cf68da

update appservice version

c7f2f56

pylint fixes

e1def2d

pylint fixes

1e1b43e

nit

f1a3804

pylint fix

41432b6

djyou reviewed Oct 19, 2018

View reviewed changes

respond to PR comments

7abc6f5

djyou approved these changes Oct 19, 2018

View reviewed changes

change from null to empty check

6005917

tjprescott approved these changes Oct 23, 2018

View reviewed changes

tjprescott merged commit 52180d1 into Azure:dev Oct 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACR: Add Pull Request Event to Task Source Trigger #7526

ACR: Add Pull Request Event to Task Source Trigger #7526

jaysterp commented Oct 9, 2018

msftclas commented Oct 9, 2018 •

edited

Loading

djyou Oct 19, 2018

djyou Oct 19, 2018

jaysterp Oct 19, 2018

jaysterp Oct 19, 2018

djyou Oct 19, 2018

jaysterp Oct 19, 2018

djyou Oct 19, 2018

djyou Oct 19, 2018

jaysterp Oct 19, 2018

djyou left a comment

jaysterp commented Oct 22, 2018 •

edited

Loading

jaysterp commented Oct 23, 2018

ACR: Add Pull Request Event to Task Source Trigger #7526

ACR: Add Pull Request Event to Task Source Trigger #7526

Conversation

jaysterp commented Oct 9, 2018

msftclas commented Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djyou left a comment

Choose a reason for hiding this comment

jaysterp commented Oct 22, 2018 • edited Loading

jaysterp commented Oct 23, 2018

msftclas commented Oct 9, 2018 •

edited

Loading

jaysterp commented Oct 22, 2018 •

edited

Loading