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

[do not merge] update OSName to OSVmName #11393

Closed
wants to merge 2 commits into from

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented May 12, 2020

Use the common script. It looks like Matrix usage isn't supported any longer.

PLAN CHANGE. This will take advantage of eng/common. This PR should not merge.

@scbedd scbedd requested review from lmazuel and mayurid as code owners May 12, 2020 20:51
@scbedd scbedd changed the title update OSName to OSVmName [do not merge] update OSName to OSVmName May 12, 2020
@@ -9,49 +9,49 @@ parameters:
BuildDocs: true
TestMatrix:
Linux_Python27:
OSName: 'Linux'
OSVmName: 'Linux'
Copy link
Member

Choose a reason for hiding this comment

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

Once we resolve Azure/azure-sdk-tools#610 (comment) we should be able to simply remove the OSName from all the matrix entries.

@weshaggard
Copy link
Member

The shared template from eng/common has just been merged #11417 so this PR can now be updated to remove OSName everywhere and use that template.

@weshaggard
Copy link
Member

fyi @chidozieononiwu

@@ -46,5 +46,5 @@ jobs:
condition: always()
inputs:
testResultsFiles: '**/junit/test-results.xml'
testRunTitle: '$(OSName) Python $(PythonVersion)'
testRunTitle: '$(OSVmName) Python $(PythonVersion)'
Copy link
Member

Choose a reason for hiding this comment

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

You should leave this as $(OSName) since it will be set by the verify-agent-os.yml script here

OSVmImage: 'ubuntu-18.04'
PythonVersion: '2.7'
CoverageArg: ''
RunForPR: true
Linux_Python35:
OSName: 'Linux'
OSVmName: 'Linux'
Copy link
Member

Choose a reason for hiding this comment

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

We are just removing this completely for all the matrix entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use it when publishing the test run title, so I'm going to keep the parameter around (just renamed).

I should just be able to remove the parameter being passed to the common yml yes?

Copy link
Member

Choose a reason for hiding this comment

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

It will be set within the script. So I think you don't need the variable at all, so long as the verify-agent-os script runs a s the first step. it is set here

@@ -78,7 +78,7 @@ jobs:
BeforeTestSteps: ${{ parameters.BeforeTestSteps }}
AfterTestSteps: ${{ parameters.AfterTestSteps }}
PythonVersion: $(PythonVersion)
OSName: $(OSName)
OSVmName: $(OSVmName)
Copy link
Member

Choose a reason for hiding this comment

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

You should just remove this parameter as its no longer required.

@@ -25,7 +25,7 @@ steps:

- template: eng/pipelines/templates/scripts/verify-agent-os.yml@azure-sdk-tools
parameters:
OSName: ${{ parameters.OSName }}
OSName: ${{ parameters.OSVmName }}
Copy link
Member

@chidozieononiwu chidozieononiwu May 13, 2020

Choose a reason for hiding this comment

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

We are now calling this without parameters, assuming that the $(OSVMImage) is already set. We are also calling it from the eng/common directory. see here , here and here

@scbedd
Copy link
Member Author

scbedd commented Sep 1, 2020

Being executed through eng/common. Closing.

@scbedd scbedd closed this Sep 1, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Nov 12, 2020
Adding API to support generating access token for ApplicationInsights Profiler (Azure#11393)

* Adds base for updating Microsoft.Insights from version preview/2020-10-05-preview to version 2020-10-26-preview

* Updates readme

* Updates API version in new specs and examples

* Update readme to pointing to profilerToken_API.json

* Update operations list

* Add profiler token getter

* Wire up the defintions

* Fix error: additonal property of liveToken

* Append post action

* Update api-version for example

* Fix some small issues

* Update readme for the resolving autorest check issue

* From profilertoken to profilerToken

* Ran prettier

* Appending back missing readme for 2020-10 tag

* Fix some mistakes

* Use common error response

* Tag secret with x-ms-secret

* Remove list operations

* Clean up packages

* Making 2 post operations for token

* Remove unused operations_list.json example

* Align with official master

* Resolve conflicts

* Add x-ms-secret for the token

* Update error response schema ref

* Update operation ids

* Remove unused error response
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.

4 participants