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

[Eng] remove need for get_test_dirs.ps1 #15874

Merged
merged 11 commits into from
Oct 20, 2021

Conversation

seankane-msft
Copy link
Member

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to CHANGELOG.md are included.
  • MIT license headers are included in each file.

@seankane-msft seankane-msft self-assigned this Oct 20, 2021
@seankane-msft seankane-msft requested review from benbp and a team as code owners October 20, 2021 15:52
@seankane-msft seankane-msft marked this pull request as draft October 20, 2021 15:52
@@ -113,8 +110,7 @@ stages:
GoWorkspace: $(GO_WORKSPACE_PATH)
Image: $(vm.image)
GoVersion: $(go.version)
RunTests: ${{ parameters.RunTests }}
TestProxy: true
TestProxy: true # Do we need this conditional if it's always true? @benbp
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need this conditional @benbp?

Copy link
Member

Choose a reason for hiding this comment

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

@seankane-msft It is needed. The value defaults to false and therefore is not specified for the live tests, samples tests, and cosmos tests. I like how it's set up as an opt-in type value right now, but if you wanted to require it to be passed in for all templates so the usage is clearer, I'd be open to that.

- pwsh: |
# ambitious_azsdk_test_proxy is the hardcoded container name used
# by the test proxy startup script
docker logs ambitious_azsdk_test_proxy
Copy link
Member

Choose a reason for hiding this comment

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

@scbedd this makes me double take every time :D

@seankane-msft seankane-msft marked this pull request as ready for review October 20, 2021 21:05
@seankane-msft seankane-msft merged commit 51e1954 into Azure:main Oct 20, 2021
@seankane-msft seankane-msft deleted the eng-simplify branch October 20, 2021 21:12
@chlowell chlowell mentioned this pull request Oct 20, 2021
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