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

exclude template from test proxy #4189

Merged
merged 2 commits into from
Dec 14, 2022
Merged

exclude template from test proxy #4189

merged 2 commits into from
Dec 14, 2022

Conversation

gearama
Copy link
Member

@gearama gearama commented Dec 14, 2022

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@gearama
Copy link
Member Author

gearama commented Dec 14, 2022

/azp run cpp - template

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gearama gearama requested a review from RickWinter December 14, 2022 02:18
@gearama
Copy link
Member Author

gearama commented Dec 14, 2022

/azp run cpp - template

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gearama gearama merged commit 7a39344 into Azure:main Dec 14, 2022
@@ -152,15 +152,15 @@ jobs:
runProxy: true
rootFolder: '$(Build.SourcesDirectory)/sdk/${{parameters.ServiceDirectory}}'
templateFolder: '$(Build.SourcesDirectory)/sdk/${{parameters.ServiceDirectory}}'
condition: and(succeeded(), contains(variables.CmakeArgs, 'BUILD_TESTING=ON'))
condition: and(succeeded(), contains(variables.CmakeArgs, 'BUILD_TESTING=ON'),ne('${{parameters.ServiceDirectory}}', 'template'))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to have someone from EngSys look at this change, before merging.

Copy link
Member

@ahsonkhan ahsonkhan Dec 14, 2022

Choose a reason for hiding this comment

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

Why do we need to exclude the template package from test proxy? And if we really need to, is this the cleanest way to do it?

We are adding exception in multiple places, which seems a bit fragile. Are these the only places that need to opt-out?

cc @danieljurek

Copy link
Member

@benbp benbp Dec 15, 2022

Choose a reason for hiding this comment

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

@gearama @ahsonkhan we also depend on the template pipeline to validate engsys changes, often ones we make that affect all repos. I would prefer not to special case template so that we can catch any possible common proxy setup issues that might affect c++.

@gearama gearama deleted the templateCI branch January 16, 2024 20:37
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