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.

Sign up for GitHub

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

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

Merged
merged 22 commits into from
Oct 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1accdca
initial work on the PR trigger
jaysterp Sep 27, 2018
20866b9
task create and update with PR trigger
jaysterp Oct 5, 2018
6b87f94
use one flag to enable commit and pr trigger events
jaysterp Oct 5, 2018
bdfd98d
nit fixes
jaysterp Oct 5, 2018
b0b3d46
Two event flags and only support one source trigger per task
jaysterp Oct 8, 2018
94d0527
PR fixes
jaysterp Oct 8, 2018
d2ddfb2
allow empty event list when disabling all trigger events
jaysterp Oct 9, 2018
7ff22c2
format fix
jaysterp Oct 9, 2018
1eb7626
do not clear event list in update, instead status disabled
jaysterp Oct 9, 2018
4961413
update task table
jaysterp Oct 9, 2018
a998801
nit fix
jaysterp Oct 9, 2018
d95e1ce
bump cli event version
jaysterp Oct 10, 2018
dbd7ced
nit fixes
jaysterp Oct 10, 2018
e40a2a5
update ACR sdk version
jaysterp Oct 18, 2018
4cf68da
version fix
jaysterp Oct 18, 2018
c7f2f56
update appservice version
jaysterp Oct 18, 2018
e1def2d
pylint fixes
jaysterp Oct 18, 2018
1e1b43e
pylint fixes
jaysterp Oct 18, 2018
f1a3804
nit
jaysterp Oct 18, 2018
41432b6
pylint fix
jaysterp Oct 19, 2018
7abc6f5
respond to PR comments
jaysterp Oct 19, 2018
6005917
change from null to empty check
jaysterp Oct 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/command_modules/azure-cli-acr/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
Release History
===============

2.1.8
+++++
* Support commit and pull request git events for Task source trigger.

2.1.7
+++++
* Fix an ACR Build encoding issue in Python2.
Expand Down
14 changes: 8 additions & 6 deletions src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ def _task_format_group(item):
('Name', _get_value(item, 'name')),
('PLATFORM', _get_value(item, 'platform', 'os')),
('STATUS', _get_value(item, 'status')),
('COMMIT TRIGGER', _get_value(item, 'trigger', 'sourceTriggers', 0, 'status')),
('SOURCE TRIGGER', _get_value(item, 'trigger', 'sourceTriggers', 0, 'status')),
('SOURCE REPOSITORY', _get_value(item, 'step', 'contextPath')),
('BRANCH', _get_value(item, 'trigger', 'sourceTriggers', 0, 'sourceRepository', 'branch')),
('BASE IMAGE TRIGGER', _get_value(item, 'trigger', 'baseImageTrigger', 'baseImageTriggerType')),
Expand All @@ -214,7 +214,8 @@ def _build_format_group(item):
('TASK', _get_value(item, 'buildTask')),
('PLATFORM', _get_value(item, 'platform', 'osType')),
('STATUS', _get_value(item, 'status')),
("TRIGGER", _get_build_trigger(_get_value(item, 'imageUpdateTrigger'), _get_value(item, 'gitCommitTrigger'))),
("TRIGGER", _get_build_trigger(_get_value(item, 'imageUpdateTrigger'),
_get_value(item, 'sourceTrigger', 'eventType'))),
('STARTED', _format_datetime(_get_value(item, 'startTime'))),
('DURATION', _get_duration(_get_value(item, 'startTime'), _get_value(item, 'finishTime')))
])
Expand All @@ -226,7 +227,8 @@ def _run_format_group(item):
('TASK', _get_value(item, 'task')),
('PLATFORM', _get_value(item, 'platform', 'os')),
('STATUS', _get_value(item, 'status')),
("TRIGGER", _get_build_trigger(_get_value(item, 'imageUpdateTrigger'), _get_value(item, 'sourceTrigger'))),
("TRIGGER", _get_build_trigger(_get_value(item, 'imageUpdateTrigger'),
_get_value(item, 'sourceTrigger', 'eventType'))),
('STARTED', _format_datetime(_get_value(item, 'startTime'))),
('DURATION', _get_duration(_get_value(item, 'startTime'), _get_value(item, 'finishTime')))
])
Expand Down Expand Up @@ -257,9 +259,9 @@ def _get_value(item, *args):
return ' '


def _get_build_trigger(image_update_trigger, git_commit_trigger):
if git_commit_trigger.strip():
return 'Git Commit'
def _get_build_trigger(image_update_trigger, git_source_trigger):
if git_source_trigger.strip():
return git_source_trigger
if image_update_trigger.strip():
return 'Image Update'
return 'Manual'
Expand Down
1 change: 1 addition & 0 deletions src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ def load_arguments(self, _): # pylint: disable=too-many-statements
# Source Trigger parameters
c.argument('source_trigger_name', help="The name of the source trigger.")
c.argument('commit_trigger_enabled', help="Indicates whether the source control commit trigger is enabled.", arg_type=get_three_state_flag())
c.argument('pull_request_trigger_enabled', help="Indicates whether the source control pull request trigger is enabled.", arg_type=get_three_state_flag())
c.argument('git_access_token', help="The access token used to access the source control provider.")
c.argument('branch', help="The source control branch name.")
c.argument('base_image_trigger_name', help="The name of the base image trigger.")
Expand Down
57 changes: 45 additions & 12 deletions src/command_modules/azure-cli-acr/azure/cli/command_modules/acr/task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def acr_task_create(cmd, # pylint: disable=too-many-locals
values=None,
source_trigger_name='defaultSourceTriggerName',
commit_trigger_enabled=True,
pull_request_trigger_enabled=True,
branch='master',
no_push=False,
no_cache=False,
Expand All @@ -77,9 +78,9 @@ def acr_task_create(cmd, # pylint: disable=too-many-locals
base_image_trigger_enabled=True,
base_image_trigger_type='Runtime',
resource_group_name=None):
if commit_trigger_enabled and not git_access_token:
raise CLIError("Commit trigger needs to be disabled [--commit-trigger-enabled False] "
"if no --git-access-token is provided.")
if (commit_trigger_enabled or pull_request_trigger_enabled) and not git_access_token:
raise CLIError("If source control trigger is enabled [--commit-trigger-enabled] or "
"[--pull-request-trigger-enabled] --git-access-token must be provided.")

if file.endswith(ALLOWED_TASK_FILE_TYPES):
step = FileTaskStep(
Expand All @@ -106,7 +107,13 @@ def acr_task_create(cmd, # pylint: disable=too-many-locals
source_control_type = SourceControlType.github.value

source_triggers = None
source_trigger_events = []
if commit_trigger_enabled:
source_trigger_events.append(SourceTriggerEvent.commit.value)
if pull_request_trigger_enabled:
source_trigger_events.append(SourceTriggerEvent.pullrequest.value)
# if source_trigger_events contains any event types we assume they are enabled
if source_trigger_events:
source_triggers = [
SourceTrigger(
source_repository=SourceProperties(
Expand All @@ -119,8 +126,8 @@ def acr_task_create(cmd, # pylint: disable=too-many-locals
scope='repo'
)
),
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,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

See fix in most recent update.

name=source_trigger_name
)
]
Expand Down Expand Up @@ -205,6 +212,7 @@ def acr_task_update(cmd, # pylint: disable=too-many-locals
timeout=None,
context_path=None,
commit_trigger_enabled=None,
pull_request_trigger_enabled=None,
git_access_token=None,
branch=None,
image_names=None,
Expand Down Expand Up @@ -282,10 +290,10 @@ def acr_task_update(cmd, # pylint: disable=too-many-locals
if task.trigger:
source_triggers = task.trigger.source_triggers
base_image_trigger = task.trigger.base_image_trigger
if commit_trigger_enabled or source_triggers is not None:
status = None
if commit_trigger_enabled is not None:
status = TriggerStatus.enabled.value if commit_trigger_enabled else TriggerStatus.disabled.value
if (commit_trigger_enabled or pull_request_trigger_enabled) or source_triggers is not None:
source_trigger_events = _get_trigger_event_list(source_triggers,
commit_trigger_enabled,
pull_request_trigger_enabled)
source_trigger_update_params = [
SourceTriggerUpdateParameters(
source_repository=SourceUpdateParameters(
Expand All @@ -297,11 +305,12 @@ def acr_task_update(cmd, # pylint: disable=too-many-locals
token_type=DEFAULT_TOKEN_TYPE
)
),
source_trigger_events=[SourceTriggerEvent.commit],
status=status,
name=source_triggers[0].name if source_triggers else "defaultBaseimageTriggerName"
source_trigger_events=source_trigger_events,
status=TriggerStatus.enabled.value if source_trigger_events else TriggerStatus.disabled.value,
name=source_triggers[0].name if source_triggers else "defaultSourceTriggerName"
)
]

if base_image_trigger_enabled or base_image_trigger is not None:
status = None
if base_image_trigger_enabled is not None:
Expand Down Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

nit: _update_trigger_event_list

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

commit_trigger_enabled=None,
pull_request_trigger_enabled=None):
source_trigger_events = set()
# perform merge with server-side event list
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()
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if commit_trigger_enabled is not None:
if commit_trigger_enabled:
source_trigger_events.add(SourceTriggerEvent.commit.value)
else:
if SourceTriggerEvent.commit.value in source_trigger_events:
source_trigger_events.remove(SourceTriggerEvent.commit.value)
if pull_request_trigger_enabled is not None:
if pull_request_trigger_enabled:
source_trigger_events.add(SourceTriggerEvent.pullrequest.value)
else:
if SourceTriggerEvent.pullrequest.value in source_trigger_events:
source_trigger_events.remove(SourceTriggerEvent.pullrequest.value)
return source_trigger_events
4 changes: 2 additions & 2 deletions src/command_modules/azure-cli-acr/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
logger.warn("Wheel is not available, disabling bdist_wheel hook")
cmdclass = {}

VERSION = "2.1.7"
VERSION = "2.1.8"
CLASSIFIERS = [
'Development Status :: 4 - Beta',
'Intended Audience :: Developers',
Expand All @@ -33,7 +33,7 @@
'azure-cli-core',
'azure-mgmt-storage==2.0.0rc4',
'azure-storage-blob==1.3.1',
'azure-mgmt-containerregistry==2.2.0',
'azure-mgmt-containerregistry==2.3.0',
]

with open('README.rst', 'r', encoding='utf-8') as f:
Expand Down
4 changes: 4 additions & 0 deletions src/command_modules/azure-cli-appservice/HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

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

Choose a reason for hiding this comment

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

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


0.2.5
+++++
* az functionapp create supports creating a linux consumption plan type with a specific runtime
Expand Down
4 changes: 2 additions & 2 deletions src/command_modules/azure-cli-appservice/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
logger.warn("Wheel is not available, disabling bdist_wheel hook")
cmdclass = {}

VERSION = "0.2.5"
VERSION = "0.2.6"
CLASSIFIERS = [
'Development Status :: 4 - Beta',
'Intended Audience :: Developers',
Expand All @@ -33,7 +33,7 @@
'azure-cli-core',
'azure-mgmt-web==0.40.0',
'azure-mgmt-storage==2.0.0rc4',
'azure-mgmt-containerregistry==2.2.0',
'azure-mgmt-containerregistry==2.3.0',
# v1.17 breaks on wildcard cert https://github.com/shazow/urllib3/issues/981
'urllib3[secure]>=1.18',
'xmltodict',
Expand Down