-
Notifications
You must be signed in to change notification settings - Fork 0
Move the ADAL automation pipeline from IDDP to Engineering #162
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
Conversation
@@ -0,0 +1,63 @@ | |||
parameters: |
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.
nit: add header. (What is this pipeline doing and what variables it use)
packageDownloadExternal: 'broker-host' | ||
versionDownloadExternal: '${{ parameters.oldBrokerHostVersion }}' | ||
- task: UniversalPackages@0 | ||
displayName: 'Download com.microsoft.windowsintune.companyportal-signed' |
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.
nit : Download Company Portal apk from feed
@@ -22,6 +22,10 @@ parameters: | |||
type: number | |||
- name: testRunTitle | |||
type: string | |||
- name: otherFiles |
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.
remove, duplicated param (see line 12)
branches: | ||
include: | ||
- dev | ||
always: true |
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.
Let's not use YML schedules. Instead, you can use the scheduled triggers feature in ADO. This way, we can diasble a broken pipeline without needing a PR to do it. Here's a recent PR i did this in: AzureAD/microsoft-authentication-library-for-android#1721
always: true | ||
|
||
parameters: | ||
- name: firebaseDeviceId |
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.
We should use oriole 32 as the default (best automation results)
- name: testTargetPackages | ||
displayName: Packages as Test Targets | ||
type: string | ||
default: package com.microsoft.identity.client.broker.automationapp.testpass, notAnnotation org.junit.Ignore |
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.
default: package com.microsoft.identity.client.broker.automationapp.testpass, notAnnotation org.junit.Ignore, notAnnotation com.microsoft.identity.client.ui.automation.annotations.DoNotRunOnPipeline
# Brokers | ||
- stage: 'brokers' | ||
dependsOn: [] # this removes the implicit dependency on previous stage and causes this to run in parallel | ||
displayName: Brokers and Azure Sample APKs |
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.
I already added a template that does this step, you can check if it would fit into this pipeline as well. download-brokers-and-azure-sample.yml.
jobs: | ||
- template: ./templates/build-broker-automation-app.yml | ||
parameters: | ||
brokerApp: AutoBroker |
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.
AutoBroker? Should this BrokerHost?
- template: ./templates/build-broker-automation-app.yml | ||
parameters: | ||
brokerApp: AutoBroker | ||
brokerFlavor: Dist |
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.
I think this should be Local
oldBrokerHostApk: brokerHost-local-debug.apk | ||
firebaseTimeout: 45m | ||
brokerApp: brokerautomationapp-dist-AutoBroker-debug.apk | ||
brokerTestApp: brokerautomationapp-dist-AutoBroker-debug-androidTest.apk |
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 these point to auto broker if this pipeline uses BrokerHost?
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.
If you do intend to use auto broker, you'll need to pull in OldAuthenticator, since Authenticator will be the broker used in the update scenario tests.
otherFiles: "/data/local/tmp/CompanyPortal.apk=$(Pipeline.Workspace)/brokerapks/$(companyPortalApk),\ | ||
/data/local/tmp/Authenticator.apk=$(Pipeline.Workspace)/brokerapks/$(authenticatorApk),\ | ||
/data/local/tmp/Authenticator.apk=$(Pipeline.Workspace)/azureSample/$(azureSampleApk),\ | ||
/data/local/tmp/BrokerHost.apk=$(Pipeline.WorkSpace)/brokerHost/$(oldBrokerHostApk)" |
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.
Otherfiles look off here. You are push BrokerHost.apk but sourcing it from $(oldBrokerHostApk)
and you are not pushing an OldBrokerHost
- template: ./templates/flank/run-on-firebase-with-flank.yml | ||
parameters: | ||
automationAppApkPath: "$(Pipeline.Workspace)/brokerautomationapks/brokerautomationapp-dist-AutoBroker-debug.apk" | ||
automationAppTestApkPath: "$(Pipeline.Workspace)/brokerautomationapks/brokerautomationapp-dist-AutoBroker-debug-androidTest.apk" |
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.
I think you can use variables brokerApp
and brokerTestApp
here
@@ -0,0 +1,63 @@ | |||
parameters: |
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.
Does this template do something different from the existing template build-broker-automation-app.yml
?
- name: otherFiles | ||
type: string | ||
- name: testsFolderName | ||
type: string |
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.
This will break other pipelines if not given a default, since they are not passing in this parameter. I would suggest either adding a default value of broker-ci
or fixing other pipelines that use this template as part of this same PR.
@@ -63,6 +65,7 @@ jobs: | |||
--environment-variables "clearPackageData=true" \ | |||
--results-history-name "${{ parameters.resultsHistoryName }}" \ | |||
--test-targets "${{ parameters.testTargetPackages }}, ${{ parameters.apiLevelTarget }}" | |||
--smart-flank-gcs-path "gs://test-lab-ffz6x9pu2y62a-is0rq7a7rwdhi/smart-flank-xml/${{ project.testsFolderName }}/JUnitReport.xml" |
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.
This is not needed when running without flank.
- name: firebaseDeviceAndroidVersion | ||
displayName: Firebase Device Android Version | ||
type: number | ||
default: 28 |
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.
32
This is one of the pipelines I have scehduled to truncate after the full implementation of the Release pipeline as it only serves a sub function of the release pipeline. |
Task: https://dev.azure.com/IdentityDivision/Engineering/_boards/board/t/Auth%20Client%20-%20Android/Backlog%20items/?workitem=1961787
Move the following pipeline from IDDP to Engineering and convert to YML
https://dev.azure.com/IdentityDivision/IDDP/_build?definitionId=1288
Also incorporate sharding and pipeline parameters as done in other pipelines here: AzureAD/microsoft-authentication-library-for-android#1683
Pipeline for review URL: https://identitydivision.visualstudio.com/Engineering/_build?definitionId=1790&_a=summary