Skip to content
This repository has been archived by the owner on Nov 20, 2023. It is now read-only.

Add buildProperties to project service configuration #383

Merged
merged 18 commits into from
Apr 23, 2020

Conversation

alexfdezsauco
Copy link
Contributor

@alexfdezsauco alexfdezsauco commented Apr 19, 2020

Add buildProperties to project service configuration.

The service configuration could look like this:

...
services:
- name: projectA
  project: ProjectA/ProjectA.csproj
  buildProperties: 
  - name: Configuration
    value: Debug

…n as global property to setup msbuild project
@davidfowl
Copy link
Member

Instead of build args it should be a dictionary of properties.

@alexfdezsauco
Copy link
Contributor Author

Ok, I will implement this with a dictionary. But there are entries in the dictionary that need to be transtaled as build command arguments. In this case, I extracted the "dictionary" only for the supported build configuration from the build args.

A question: What to do with the all properties in the dictionary? I know what to do with the build Configuration. But the others?

@davidfowl
Copy link
Member

Ok, I will implement this with a dictionary. But there are entries in the dictionary that need to be transtaled as build command arguments. In this case, I extracted the "dictionary" only for the supported build configuration from the build args.

They can all be translated via /p:{Key}={Value}, it's just msbuild properties. You can choose to recognize some as first class but I wouldn't at the moment.

@alexfdezsauco alexfdezsauco changed the title Add buildArgs to service configuration and capture build configuration Add service properties to service configuration and capture build configuration Apr 19, 2020
@alexfdezsauco
Copy link
Contributor Author

I tried something different for the moment, added properties but only build Configuration as first class

@alexfdezsauco
Copy link
Contributor Author

Also translated extra properties as /p:{Key}={Value}

@alexfdezsauco alexfdezsauco changed the title Add service properties to service configuration and capture build configuration Add service properties to service configuration and capture as build parameters Apr 19, 2020
@davidfowl
Copy link
Member

This change needs a test, my guess is that this will fail with --docker as well.

…rty>) of ConfigService property to load build properties
@alexfdezsauco
Copy link
Contributor Author

This change needs a test, my guess is that this will fail with --docker as well.

Will check this one.

@alexfdezsauco
Copy link
Contributor Author

This change needs a test, my guess is that this will fail with --docker as well.

Added support for --docker option, working on the test

@rynowak rynowak linked an issue Apr 20, 2020 that may be closed by this pull request
@alexfdezsauco alexfdezsauco changed the title Add service properties to service configuration and capture as build parameters Add buildProperties to project service configuration Apr 21, 2020
Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

This looks good. A few nits.

{
UseMultiphaseDockerfile = false,
};
project.ContainerInfo = new ContainerInfo() { UseMultiphaseDockerfile = false, };
Copy link
Member

Choose a reason for hiding this comment

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

The reformatting changes make it really hard to tell what actually changed. In the future when you send more PRs (and please send more PRs these are great) I'd refrain from doing these changes as they are a bit distracting in the diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before commit I'm running 'dotnet format -w.'. I don't remember changing that line explicitly. I'll be more careful next time.

Copy link
Member

Choose a reason for hiding this comment

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

No problem. Your contributing some great stuff! Keep it coming!

@davidfowl davidfowl merged commit d429d0b into dotnet:master Apr 23, 2020
kishanAnem pushed a commit to kishanAnem/tye that referenced this pull request May 15, 2020
* Add buildArgs to service configuration and capture build configuration as global property to setup msbuild project

* Fix format

* Fix - Error CS8600: Converting null literal or possible null value to non-nullable type.

* Improve configuration format for build arguments

* Fix whitespace format

* Fix error CS8618: Non-nullable property 'Properties' is uninitialized. Consider declaring the property as nullable.

* Fix error CS8601: Possible null reference assignment.

* Translate non first class properties as /p:{Key}={Value} into the build command

* Fix property translation

* All properties are used to create the msbuild project

* Change the name (to BuildProperties) and the type (to List<BuildProperty>) of ConfigService property to load build properties

* Add support of build properties when tye run with --docker option

* Fix ComprehensionalTest

* Add tests to verify the output directory for the corresponding build configuration

* Fix whitespace format

* Override the correct CreateTestCasesForTheory to fix error CS0618

* Remove the usage of BuildPropertiesToOptionsMap and fix the code format
kishanAnem pushed a commit to kishanAnem/tye that referenced this pull request May 15, 2020
* Add buildArgs to service configuration and capture build configuration as global property to setup msbuild project

* Fix format

* Fix - Error CS8600: Converting null literal or possible null value to non-nullable type.

* Improve configuration format for build arguments

* Fix whitespace format

* Fix error CS8618: Non-nullable property 'Properties' is uninitialized. Consider declaring the property as nullable.

* Fix error CS8601: Possible null reference assignment.

* Translate non first class properties as /p:{Key}={Value} into the build command

* Fix property translation

* All properties are used to create the msbuild project

* Change the name (to BuildProperties) and the type (to List<BuildProperty>) of ConfigService property to load build properties

* Add support of build properties when tye run with --docker option

* Fix ComprehensionalTest

* Add tests to verify the output directory for the corresponding build configuration

* Fix whitespace format

* Override the correct CreateTestCasesForTheory to fix error CS0618

* Remove the usage of BuildPropertiesToOptionsMap and fix the code format
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support doing tye run with release assets.
2 participants