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

Cake Frosting should respect configuration #3048

Merged
merged 1 commit into from Jan 24, 2021
Merged

Cake Frosting should respect configuration #3048

merged 1 commit into from Jan 24, 2021

Conversation

patriksvensson
Copy link
Member

This pull request makes sure that Cake.Frosting respect any user configuration related to the tool installation path.

@@ -152,7 +153,7 @@ public static CakeHost UsePackageInstaller<TPackageInstaller>(this CakeHost host
/// <param name="host">The <see cref="CakeHost"/> to configure.</param>
/// <param name="uri">The tool URI.</param>
/// <returns>The same <see cref="CakeHost"/> instance so that multiple calls can be chained.</returns>
public static CakeHost UseTool(this CakeHost host, Uri uri)
public static CakeHost InstallTool(this CakeHost host, Uri uri)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. I'm assuming v1.0 we're OK with breaking public APIs

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we're already breaking some APIs 😅

Copy link
Member

Choose a reason for hiding this comment

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

We should create an additional issue for it though so that it will be listed in release notes.

Copy link
Member

Choose a reason for hiding this comment

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

@augustoproiete
Copy link
Member

I tested this one locally on my machine, and everything works (in the sense that tools are installed), but it doesn't honor the Tools Path in cake.config. The tools are being installed in a tools folder, but my cake.config tells it to use a .cake folder.

I tried:

  • Having a single cake.config side-by-side with the Frosting build.csproj
  • Having a single cake.config at the root of the project

Test code is on this branch:
https://github.com/augustoproiete-forks/cake-build--cake/tree/feature/GH-2904-test1/src/build

@patriksvensson
Copy link
Member Author

Frosting does not support cake.config.

@gep13
Copy link
Member

gep13 commented Jan 24, 2021

Based on the comments in the associated issue, it seems that there is an expectation that the cake.config file will be honoured.

@patriksvensson
Copy link
Member Author

Ok, then we need to add support for cake.config which has never been supported in Frosting before, and I don't think it should tbh.

@gep13
Copy link
Member

gep13 commented Jan 24, 2021

@patriksvensson the title of the issue that this PR is associated with is:

(Frosting) Tool installer should respect configuration

If not cake.config, what configuration is this issue referring to? There isn't any details in the issue.

@patriksvensson
Copy link
Member Author

Yeah, maybe I'm wrong here.

Copy link
Member

@pascalberger pascalberger left a comment

Choose a reason for hiding this comment

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

LGTM

@pascalberger pascalberger linked an issue Jan 24, 2021 that may be closed by this pull request
@pascalberger
Copy link
Member

This fixes #3038. I removed link to #2904 since it does not add support for configuration and this is something which we decided to move post 1.0.

@pascalberger pascalberger merged commit 9570b86 into cake-build:release/1.0.0 Jan 24, 2021
@pascalberger
Copy link
Member

@patriksvensson your changes have been merged, thanks for your contribution 👍

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.

Tool resolving in Frosting tasks
4 participants