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

Support for Properties argument in nuget pack #666

Closed
daveaglick opened this issue Jan 27, 2016 · 18 comments · Fixed by #681
Closed

Support for Properties argument in nuget pack #666

daveaglick opened this issue Jan 27, 2016 · 18 comments · Fixed by #681
Milestone

Comments

@daveaglick
Copy link
Member

When packing a project file (#637), the -Properties argument is often used to specify the build configuration (or perform substitution in an adjacent nuspec):

nuget pack foo.csproj -Build -Symbols -Properties Configuration=Release

Support for specifying multiple properties should be added to NuGetPackSettings and NuGetPacker.GetArguments(). The generated argument should separate multiple properties with a semicolon per the NuGet command line reference.

@daveaglick
Copy link
Member Author

Code is done, but not tests. Holding off on tests and submitting a PR until #662 and #667 are merged to avoid submitting a PR based on a PR based on a PR. The tests for this one will be based on project packing tests from #667 since the -Properties argument is only used for packing project files.

@gep13
Copy link
Member

gep13 commented Jan 27, 2016

👍 just thinking about this one as well, so that an alternative is mentioned...

You could use the ArgumentCustomization method on the base ToolSettings class to extend the arguments that are supported by an alias.

@daveaglick
Copy link
Member Author

@gep13 You mean as a user (I.e., if I wanted to get the Properties argument into the settings today without any code changes)? Would that work since NuGetPacker.GetArguments() doesn't ever evaluate ToolSettings.ArgumentCustomization? Perhaps the following should also be added to NuGetPacker.GetArguments():

if (settings.ArgumentCustomization != null)
{
    builder = settings.ArgumentCustomization(builder);
}

@gep13
Copy link
Member

gep13 commented Jan 27, 2016

Assuming that NuGetTool derives from Tool, which I assume it does, then the base class has this covered:

if (settings.ArgumentCustomization != null)

No?

@gep13 gep13 closed this as completed Jan 27, 2016
@gep13 gep13 reopened this Jan 27, 2016
@gep13
Copy link
Member

gep13 commented Jan 27, 2016

@patriksvensson did you mean to close this issue?

@gep13
Copy link
Member

gep13 commented Jan 27, 2016

I didn't mean to close it, that was an accident on my part, but then I clicked re-open, and immediately it got closed by you 😄

@daveaglick
Copy link
Member Author

Hahaha - "THIS IS THE WORST IDEA EVAR!!!! CLOSE!!!!"

@daveaglick
Copy link
Member Author

@gep13

the base class has this covered

Indeed it does! I wasn't following the flow close enough and didn't notice that NuGetPacker.GetArguments() actually returns a ProcessArgumentBuilder that gets passed to the process runner.

@daveaglick
Copy link
Member Author

It's good to know about the ability to customize the settings arguments - that'll come in handy.

I still think the explicit Properties dictionary is warranted since it makes things a little clearer (especially with the slightly goofy key=value;key=value argument syntax).

@gep13
Copy link
Member

gep13 commented Jan 27, 2016

@daveaglick said...
I still think the explicit Properties dictionary is warranted since it makes things a little clearer

oh yeah, absolutely, I am not suggesting that this isn't needed. I just wanted to make sure that you were aware of the option to do it using the other technique, in the interim.

@daveaglick
Copy link
Member Author

👍

@patriksvensson
Copy link
Member

@daveaglick @gep13 Seriously, I have no idea why this issue got closed. I've been at a dinner tonight and only looked at the issue on my phone when I got a notification...

@gep13
Copy link
Member

gep13 commented Jan 27, 2016

@patriksvensson so did you re-open it as well? If not, maybe a good time to change your GitHub password 😢

@devlead
Copy link
Member

devlead commented Jan 27, 2016

Have you checked the issue number...👿

@patriksvensson
Copy link
Member

@gep13 Yes, I reopened the issue manually when I saw your conversation and I have two factor auth.

@daveaglick
Copy link
Member Author

So tempted to close and immediately reopen this again just to mess with everyone...

@patriksvensson
Copy link
Member

@daveaglick lol 😄

@Philo
Copy link
Contributor

Philo commented Feb 11, 2016

@patriksvensson, any chance this (#681) could be included in milestone v0.9.0 also. This is closely related to #637 in terms of features that will be utilised together.

For example, I'm looking to iterate through projects in a solution containing nuspec files, then do a "Nuget pack" against the project file and pass the configuration as a property to nuget to determine the files to pack:

nuget pack MyProject.csproj -Version 1.0.0 -Properties Configuration=Release -OutputDirectory ./artifacts -IncludeReferencedProjects

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants