Revert too eager deletion of the EnvironmentName config setting #296
Conversation
|
Question for @ferventcoder: Is the configuration property part of the API surface area or is it something that we just use internally. If it's just internal, then there is no need for this P/R to revert the changes. |
|
Looks like we can actually skip this "repair" pull request, yes, @BiggerNoise. In There are also tests on this (not testing the command-line switches, but the configuration properties themselves): (from But I would be more comfortable is @ferventcoder could confirm it too :) |
| if (!string.IsNullOrEmpty(configuration_property_holder.EnvironmentName)) | ||
| configuration_property_holder.EnvironmentNames = configuration_property_holder.EnvironmentName; | ||
| else | ||
| configuration_property_holder.EnvironmentNames = ApplicationParameters.default_environment_name; |
There was a problem hiding this comment.
No matter what, this section needs to use the new name - think this was an oversight in the original PR (that moved to env names)
There was a problem hiding this comment.
There is a line above, @ferventcoder. This is only run if the plural one is not set. So it is effectively (pseudo-code):
environmentNames == environmentNames ?? environmentName ?? defaultEnvironmentName
| @@ -40,6 +40,8 @@ public interface ConfigurationPropertyHolder | |||
| string VersionTableName { get; set; } | |||
| string ScriptsRunTableName { get; set; } | |||
| string ScriptsRunErrorsTableName { get; set; } | |||
| [Obsolete("Use EnvironmentNames")] | |||
| string EnvironmentName { get; set; } | |||
There was a problem hiding this comment.
This may be able to be set from the RoundHousE api, so best to check if you can property config in first
There was a problem hiding this comment.
I didn't quite understand what you mean. I have effectively just reverted to "old" (as in original EnvironmentName -> EnvironmentNames PR) behaviour. Should we make "EnvironmentName" a Facade for EnvironmentNames instead, maybe? (i.e. write-through on set, read from EnvironmentNames on read?)
There was a problem hiding this comment.
I think EnvironmentName is effectively a facade now
There was a problem hiding this comment.
it could probably use a little bit more to make that change effective.
There was a problem hiding this comment.
Yes, maybe. So we might as well just make it a proper facade in ConfigurationPropertyHolder, and move the "if empty then the other one, else this" logic there, or remove it altogether (as in the original trigger-happy PR). What do you prefer?
I prefer making EnvironmentNames a list (of some sorts) of string, and make conversion (split on comma or whatever) on reading command-line. It makes more sense when working with RoundhousE as a library. Unbound strings are evil. You can store anything from a single character to the complete works of Shakespeare in them, and there's no way of telling from the signature if there is a "hidden format" you should use (e.g. comma-separated strings).
There was a problem hiding this comment.
I will have a closer look.
There was a problem hiding this comment.
Agree that the property should be a string collection and that it should be split when the command line is read. Parsing a comma separated string provided as an API argument is just silly.
We can have the obsoleted singular property that is just a facade for the collection; agree with that approach.
All of this code is present, just a matter of some reorganization. I'm busy tonight, but I can definitely put this together Wednesday night. If you want to do this one, feel free, but I definitely don't mind jumping in on some code (build systems bore me to tears...yes, they're really important, but I'd rather watch a wall of paint dry.)
There was a problem hiding this comment.
Lovely if you can have a look, @BiggerNoise. I'd feel more comfortable on this one if we have more than two eyes on it, as we've had some discussions around it. Can you technically commit to/continue on the same pull request, how does that work?
There was a problem hiding this comment.
I'll start a new P/R based upon your work here. We can create a branch in this repo if we get into more complicated things, but I don't think that's warranted here.
There was a problem hiding this comment.
Perfect. I Agree, if we have many people working on the same thing, a branch is better. But not necessary here.
|
Program.cs is when it sets command line only values. With RH you need to consider other client access, lile the rh api, or the MSBuild task. |
|
Closing in favor of #297 which includes this work as well as the changes to the implementation discussed here. |
This reverts a bit too eager deletion of the
EnvironmentNameconfiguration property, from pull request #295.