(GH-1313) Allow properties to be passed to 'pack' #1314

heaths · 2017-05-27T18:59:39Z

Passes properties through to the nuget pack command to replace variables
in the nuspec file.

Note that I have no yet cleared the CLA. I need to run this by our legal department but any preliminary feedback would be appreciated so the PR is ready to merge. Given it's a holiday weekend and I'm out on vacation next week, I may not be able to sign the CLA for a week or two. (I'm in the office one day next week so maybe I can get clearance if our legal reps are in.)

Closes #1313

ferventcoder · 2017-05-27T19:18:23Z

src/chocolatey/infrastructure.app/commands/ChocolateyPackCommand.cs

+
+                properties.Add(pair[0], pair[1].remove_surrounding_quotes());
+            }
+        }


@heaths there is already a pattern used in ChocolateyNewCommand - let's keep this consistent with that if possible

In other words, there is a function for handling parsing anything that is unparsed

I saw that, but not sure how this fits that pattern. These aren't unparsed. I have to split the string passed to --properties.

I see. For consistency with Chocolatey, I think these would just be passed in as key=value key2=value2 and wouldn't need a --properties bit. It matches consistency with choco, even if it is not consistent with nuget. I need to think some on this as there are advantages to either direction.

Okay. I looked for prior art (I'm a fan of project codebase consistency - just ask my teammates when I review most of our PRs) but didn't think to look at install. I didn't see that in usage output, though. Obviously (I think) this isn't like --params which I'm assuming at face value just passes the string through to the main install program as-is. But looking at ChocolateyInstallCommand.handle_additional_argument_parsing, would all name/value pairs be passed as --foo=1 --bar=baz. I guess I could see that as an alternative but - at least for the current install command help - that doesn't seem as obvious. I could update the documentation for both install and pack if you'd rather go that way. I can queue up the other requested changes for now.

New, look at ChocolateyNewCommand, not install. :)

Here's the area I was talking about:

choco/src/chocolatey/infrastructure.app/commands/ChocolateyNewCommand.cs

Lines 86 to 103 in 237de6a

foreach (var unparsedArgument in unparsedArguments.or_empty_list_if_null())

{

var property = unparsedArgument.Split(new[] { '=' }, 2, StringSplitOptions.RemoveEmptyEntries);

if (property.Count() == 2)

{

var propName = property[0].trim_safe();

var propValue = property[1].trim_safe().remove_surrounding_quotes();

if (configuration.NewCommand.TemplateProperties.ContainsKey(propName))

{

this.Log().Warn(() => "A value for '{0}' has already been added with the value '{1}'. Ignoring {0}='{2}'.".format_with(propName, configuration.NewCommand.TemplateProperties[propName], propValue));

}

else

{

configuration.NewCommand.TemplateProperties.Add(propName, propValue);

}

}

}

Also note how this gets changed in the help menu section as well -

choco/src/chocolatey/infrastructure.app/commands/ChocolateyNewCommand.cs

Line 129 in 237de6a

choco new <name> [<options/switches>] [<property=value> <propertyN=valueN>]

:

[<property=value> <propertyN=valueN>]

We'd also likely add at least one example in the help menu section.

Okay. Maybe I'll break that out into a common function. Could be an extension method itself in perhaps StringExtensions?

ferventcoder · 2017-05-27T19:20:08Z

src/chocolatey/infrastructure.app/configuration/ChocolateyConfiguration.cs

+    {
+        public PackCommandConfiguration()
+        {
+            Properties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);


I'm not sure what to make of this comment. Please clarify.

Thumbs up? Just means it looks good.

I get that. Not used to see it in a PR (we generally use comments for either asking about code or asking for a change). I have our internal build defs set up to even require resolving all comments one way or another, so it just seemed a bit odd.

Ah fair - we like to tell folks the things we like as well as what we see that we that has questions, comments, or needs changes. Some things come down to differences in preferences and don't require any changes, just comments.

ferventcoder

I'm not sure where this gets passed through to NuGet.Core, but reviewing this from my phone. Overall so far so good. A couple of this to change.

ferventcoder · 2017-05-27T19:23:55Z

src/chocolatey.tests.integration/scenarios/PackScenarios.cs

+
+                Scenario.reset(Configuration);
+
+                Configuration.PackCommand.Properties.Add("version", "0.1.0");


Strange that you added version - it is already a first class citizen - should --version 1.2.3 override version=1.2.3 if both are specified?

That might be best. Seems nuget does it that way as well.

ferventcoder · 2017-05-27T19:27:39Z

src/chocolatey/infrastructure.app/services/NugetService.cs

+            // Add any other properties passed to the pack command overriding any present.
+            foreach (var property in config.PackCommand.Properties)
+            {
+                this.Log().Debug(() => "{0} property '{1}': {2}".format_with(


Debug, Verbose, or Trace here? I think Debug is fine.

Any concerns about sensitive data being exposed here? I will need to check if we log these in new.

The properties passed to the command line are already dumped in Debug. Everything in the command configuration is. I figured 1) this helps me test the feature, and 2) might help someone figure out why some properties was overwritten. Using version above was more a means to an end to test this behavior (real-world scenario might be someone having built up the value passed to --properties and one value overwrites another - just like with MSBuild properties). But if you want --version to always win, I can certainly change that and modify the test to pass the same variable name twice.

I also figured Debug because I could only see this useful to figure out what a property was overwritten (and, again, to more easily test the feature without having to crack open the nupkg and check the manifest...or do you already have helpers to do that?). Trace maybe, but seemed too low-level for Verbose.

With Chocolatey, Trace is the lowest level - verbose is a separate thing that lets you log to debug or any other level, but only on verbose logger. So you could log something at info level to verbose, and it wouldn't show up without adding the verbose switch.

I see. I spent some time grokking the code base but focused more on areas I needed to update. So would you prefer Trace or Debug?

The log file contains everything debug/verbose by default - it does not log trace anywhere unless specifically requested. Trace is new, and I have some specific fixes that make the above true in a branch not yet pushed back up, so I might have you rebase your changes once that is in. We need some more docs on the levels.

As for debug logging in other places, typically a parameter that has "password" in the name will cause the logging of the passed in parameters to stop logging that data.

Let's keep it as is for now

heaths · 2017-06-14T08:04:32Z

@ferventcoder the CLA is signed and all tests fixed (seems test.bat doesn't run integration tests so I missed a break, but was able to reproduce the same command AppVeyor uses).

heaths · 2017-07-05T21:55:04Z

I'd love to be able to add Chocolatey-specific properties to my vswhere package but that will require this change so I can pass in a couple of variables in my build. Anything else you require for changes?

ferventcoder · 2017-07-05T22:07:01Z

Howdy - thanks for the reminder - this looks good, and will go into 0.10.8 (barring any high priority fixes).

ferventcoder · 2017-07-06T19:58:14Z

One note @heaths - the commits seem to be all about the same thing, so I can squash the commits down or you can. It will still have you as the author if I do it. Up to you.

Passes properties through to the nuget pack command to replace variables in the nuspec file.

ferventcoder · 2017-08-06T23:47:18Z

Pulling this in now.

ferventcoder · 2017-08-06T23:53:38Z

Merged into stable at 82224f1. This will be included in 0.10.8, thank you very much for your contribution!

ferventcoder reviewed May 27, 2017

View reviewed changes

ferventcoder requested changes May 27, 2017

View reviewed changes

ferventcoder approved these changes Jul 5, 2017

View reviewed changes

(chocolateyGH-1313) Allow properties to be passed to 'pack'

9da1a11

Passes properties through to the nuget pack command to replace variables in the nuspec file.

heaths force-pushed the issue1313 branch from bfd0381 to 9da1a11 Compare July 6, 2017 21:31

ferventcoder closed this Aug 6, 2017

ferventcoder mentioned this pull request Nov 3, 2017

AppVeyor fails when doing Choco Pack: #1402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(GH-1313) Allow properties to be passed to 'pack' #1314

(GH-1313) Allow properties to be passed to 'pack' #1314

heaths commented May 27, 2017 •

edited by ferventcoder

Loading

ferventcoder May 27, 2017

ferventcoder May 27, 2017

heaths May 28, 2017

ferventcoder May 28, 2017

heaths May 28, 2017

ferventcoder May 28, 2017

ferventcoder May 28, 2017 •

edited

Loading

ferventcoder May 28, 2017

ferventcoder May 28, 2017

heaths May 29, 2017

ferventcoder May 27, 2017

heaths May 28, 2017

ferventcoder May 28, 2017

heaths May 28, 2017

ferventcoder May 28, 2017

ferventcoder left a comment

ferventcoder May 27, 2017

heaths May 28, 2017

ferventcoder May 27, 2017

heaths May 28, 2017 •

edited

Loading

ferventcoder May 28, 2017

heaths May 28, 2017

ferventcoder May 28, 2017

heaths commented Jun 14, 2017

heaths commented Jul 5, 2017

ferventcoder commented Jul 5, 2017

ferventcoder commented Jul 6, 2017

ferventcoder commented Aug 6, 2017

ferventcoder commented Aug 6, 2017

	foreach (var unparsedArgument in unparsedArguments.or_empty_list_if_null())
	{
	var property = unparsedArgument.Split(new[] { '=' }, 2, StringSplitOptions.RemoveEmptyEntries);
	if (property.Count() == 2)
	{
	var propName = property[0].trim_safe();
	var propValue = property[1].trim_safe().remove_surrounding_quotes();

	if (configuration.NewCommand.TemplateProperties.ContainsKey(propName))
	{
	this.Log().Warn(() => "A value for '{0}' has already been added with the value '{1}'. Ignoring {0}='{2}'.".format_with(propName, configuration.NewCommand.TemplateProperties[propName], propValue));
	}
	else
	{
	configuration.NewCommand.TemplateProperties.Add(propName, propValue);
	}
	}
	}


		Scenario.reset(Configuration);

		Configuration.PackCommand.Properties.Add("version", "0.1.0");

(GH-1313) Allow properties to be passed to 'pack' #1314

(GH-1313) Allow properties to be passed to 'pack' #1314

Conversation

heaths commented May 27, 2017 • edited by ferventcoder Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferventcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heaths May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heaths commented Jun 14, 2017

heaths commented Jul 5, 2017

ferventcoder commented Jul 5, 2017

ferventcoder commented Jul 6, 2017

ferventcoder commented Aug 6, 2017

ferventcoder commented Aug 6, 2017

heaths commented May 27, 2017 •

edited by ferventcoder

Loading

ferventcoder May 28, 2017 •

edited

Loading

heaths May 28, 2017 •

edited

Loading