environmentName should be cached before modifying project properties

[Vampire]

https://github.com/stevesaliman/gradle-properties-plugin/blob/master/src/main/groovy/net/saliman/gradle/plugin/properties/PropertiesPlugin.groovy#L93 should not query project.environmentName, but use the environmentName read out before processing all the properties and stored in a local variable.

In the current version the following can happen:

• environmentName is "local"
• No gradle-local.properties is present
• <gradleUserHomeDir>/gradle-<gradleUserName>.properties contains a property "environmentName" with value "foo"

Now, while environmentName used was "local" and there it was legal that the file was not present, the build nevertheless fails with "No environment files were found for the 'foo' environment"

[Vampire]

Well, while thinking about it and playing with it I came to the conclusion that it is not the right tactic. Because then it does work for the one plugin, but the changed environmentName will be used for subprojects which is unnice I think, because then you use different environments on different levels and also this could get problematic with Gradle features as configure-on-demand. So I think the better solution would be to disallow entries for environmentName and gradleUserName that would change the current value.

[stevesaliman]

I think we still have a logic problem with the way this works. Consider the following scenario:

1. environmentName is set in gradle.properties - this is legal, and a standard Gradle location for a property name.
2. environmentName is overridden on the command line. This is also a legal standard location for a property name.

In this case, the plugin would use the value given on the command line (correctly), but fail when it processes the gradle.properties file because it detects a change to the environmentName. I think this is probably bad.

I think the only time we have an issue is when we set a value for the environmentName in a place that Gradle doesn't normally look, and that place is the last place in the chain, as in your original example where no environment name was known at apply time, or when the gradle.properties contains environmentName=dummy and gradle-dummy.properties contains environmentName=somethingElse. In all other cases, the last one in wins, just as it always did.

I could change it so that it only checks for changing the environment name property from the 2 non-standard files, but that still leaves a problem when a file is set in gradle-dummy.properties and overridden on the command line. In this case, we should allow it, since the correct value would win in the end when we re-process command line arguments.

The solution may be to keep track of the last place where the environment name property and user name property is set. If it turns out to be one of the plugin-only files, then, and only then, would we fail the build. I'll try to look at that in the morning.

[stevesaliman]

After thinking about this a bit more, I have a better idea:

I can keep track of the environment and user property values that were in effect before the plugin was applied, and compare it to the values after the plugin was applied. If they are different, I can do one of 2 things:

1. I can fail the build
2. I can simply change the values back to what they were at the start so that child builds get the right value.

I'm thinking of option 1, since it would give the user a heads up that there is a conflict.

[Vampire]

You are totally right. That was bullshit of course. I like your last idea and just implemented it, see the new PR #12 :-)

[stevesaliman]

The more I think about this, the more I think that there isn't a good solution for this issue.

At the heart of this issue, all we are really trying to do is make sure that all projects in a multi-project build are using the same value for the environmentName property. But there is no way for a child project to know what environment was used by the parent project because the properties plugin is applied independently in each project, unless we do something extremely complicated. And Gradle allows child projects to have a different value for a property than the parent. Anything we do to try to resolve this plugin issue contradicts that policy.

This means that environmentName could have one value in the parent project's gradle.properties file, and a different value in the child project's gradle.properties file. Gradle says that this is a perfectly acceptable way of defining a property, and I don't think the environmentName property should work any differently.

Then I started thinking about how the properties plugin was designed to work. I always envisioned the environmentName property coming from the command line. This would result in all projects using the same value since it has the highest precedence over all other ways of defining a property. If the environmentName is not set, the plugin assumes "local", so the only reason a users would put an environmentName in a file is if they wanted to specify a different default value for the environment. The environment specific files and the child gradle.properties are bad places to put a default value for a multi-project build.

This leaves 2 use cases I can think of:

1. The user wants to use a different default value for environmentName than "local". In this case, the default environment should go in the parent project's gradle.properties file, or the user's $HOME/.gradle/gradle.properties file if the new default should apply for all projects.
2. environmentName conflicts with a property already in use by the user's project. Issue #9 takes care of that by allowing the user to specify a different property for the plugin's environment name.

I'm going to revert the changes we've made in our attempts to make the environment name immutable. I hope to have a release done in the next couple of days.

[Vampire]

But the change we made supports those use-cases. As you noted, it does not really make sense to change the environmentName or the gradleUserName. And this change here does prevent exactly that. It is, so to say, a help for the user to not make a stupid configuration by accident or intentionally.

Also, without this check I think you will break features like configure-on-demand, or at least allow really strange behaviour to happen. Imagine three projects A, B and C. C is subproject of B, B is subproject of A.

Now if do not use configure-on-demand and project C is built, first A is configured, then B, then C. B will change the environmentName or gradleUserName which is set as project property. C will then use that changed environmentName or gradleUserName to retrieve the properties.

Now if you DO use configure-on-demand and run the same build, first A is configured as it is the root project, then C. As B was not configured, it didn't change the environmentName or gradleUserName. C will use the original environmentName or gradleUserName to retrieve the properties and thus will have different properties set which is bad and a very subtle bug in ones build.

If you really insist on removing this security net, I'd suggest to at least add my original request in this issue.

[stevesaliman]

The way I understand Gradle, the changes we've made does nothing to stop Project C from using a different environment from Project A, so the code we had added complexity without resolving the initial problem.

Configuring the property names the plugin uses is still in the code (a really great idea), as is the code that reads the environment name at the beginning so that it gets consistently used regardless of what happens as the plugin runs.

I think I need to get a better understanding of configure-on-demand to see how that effects all of this - both with the plugin, and properties in general.

It seems like there is still work to be done here, so I'm going to re-open this issue.

[Vampire]

Ah, right, I forgot I added the envName local variable with the configurable property names.

You are right that you can set a different environmentName in gradle.properties of C than was set in A and that this will be used then. But this is the standard Gradle behaviour and should not influence configure-on-demand, otherwise the implementation of the feature would be broken.

The point is that your plugin runs at configure time and modifies the project properties. So if you modify environmentName in the gradle-local.properties file of B, you will get a different result if you use configure-on-demand as I described above. The code we came up with would prevent this, as it prevents the plugin specific logic - and only the plugin specific logic - from modifying those properties.

[stevesaliman]

I guess what I'm having trouble understanding is why a user would set the environmentName the plugin should use in a file that only gets read after the plugin runs. The whole point of making the name of the environment property configurable is to avoid conflicts with other properties in the build.

Most users will set the environment name on the command line, which avoids this issue completely. Most of the rest will set it in the root project's gradle.properties, or their own .gradle/gradle.properties. I'm not sure it makes sense to add added complexity, with possible side effects for a use case that will rarely happen.

[Vampire]

A user could set it by accident, or because of a property naming clash he didn't recognize yet. e. g. if a user adds the properties plugin and doesn't remember or know that environment name is used somewhere and then someone puts environment name in his gradle-local.properties file, it makes boom. I agree that it should happen realtively rarely, but I always like my code to be as solid as possible and this check was to make the properties plugin more solid against mistakes, stupididity or plain evilness. I don't think this check will have unwanted side effects, it just ensures that the plugin logic does not change those properties.

[stevesaliman]

The challenge will be to complain when the plugin's environment property is changed in the plugin-only files, and only in the plugin-only files, and only if the change is meaningful - the change isn't overridden later by a command line property for example.

I get that a user could set it by accident, but I'm guessing that user will only do it once :-)

My main concern is that implementing this correctly is non-trivial, and adds complexity to the code base, which must be maintained. I'm trying to weigh this added complexity against the chances of this actually happening and the damage that could happen when it does happen.

[Vampire]

The challenge will be to complain when the plugin's environment property is changed in the plugin-only files, and only in the plugin-only files, and only if the change is meaningful - the change isn't overridden later by a command line property for example.

That is exactly what my last code did. Before the plugin runs, the Gradle specific logic applied and had an outcome. This is saved. Then the plugin specific logic is applied. If now the outcome is different, the plugin specific logic changed the value. As this check was at the very end after processing all including environment variables, system properties and comman dline properties, the last part of your requirement also applied.

I get that a user could set it by accident, but I'm guessing that user will only do it once :-)

You never know. You sometimes make errors multiple times, with some significant amount of time inbetween. :-) Also each user can it do once or more. g The problem is that the problems could be as subtle as the difference in using configure-on-demand or to not use it which should not alter the outcome of a build. And I think it is better to help the user here by preventing him from doing something stupid.

My main concern is that implementing this correctly is non-trivial, and adds complexity to the code base, which must be maintained. I'm trying to weigh this added complexity against the chances of this actually happening and the damage that could happen when it does happen.

Do you really think those 10 lines of code of me were non-trivial and complex? I thought the last solution was quite simple and straight forward and - opposed to the solution before - works as intended and does not block normal gradle-standard property overriding techniques.

[stevesaliman]

The 10 lines of code were pretty simple - but the unit tests to make sure the plugin works as expected with all the various ways a property can be set is another story.

1. I have to have tests that change the property in standard ways and make sure they are allowed.
2. I have to have tests that change the property in non standard ways, but are changed again later and make sure those are also allowed.
3. I have to have tests that change the property in non standard ways and leave it changed and make sure it fails.
4. I have to have tests that change the property in non standard ways, but to the value the plugin started with.

And those are just off the top of my head. I'm sure there are other situations I'd also need to test. Then I'd need to add similar tests for the user name property. Then I need to consider what would happen in a multi-project build to make sure those all work properly, only now there are more possibilities for how to set or change a property.

And I still need to get more familiar with Configuration On Demand to see how that effects things.

If I seem reluctant, it is because this feels like it is a lot of work for something that probably won't happen very often.

[Vampire]

Yeah, ok, the test are more non-trivial, that's right. So lets just leave out the tests, those 10 lines are straight forward enough to just be happy with the existing tests. :-D

[stevesaliman]

If we are going to fail a build because of something the user has done, I think we need to have tests to make sure the build only fails when we want it to fail.

Since we can't prevent a parent and child from using different values, I'm still thinking we should leave this alone, and what the user does, the user does.

[Vampire]

Your decision, it is your project. But I still think with things like configure-on-demand in mind it is better to help the user not do a stupid mistake that will take him ages to find out. Or maybe he does not find out what is wrong and just thinks either configure-on-demand is broken or the properties plugin is broken as configure-on-demand does not work with it applied correctly.

[stevesaliman]

There is some good functionality in 1.4.0, and I don't want to hold up its release for this enhancement, which I'm still not sure about.

I'm going to go ahead and release 1.4.0, then I'll start to look into configure-on-demand some more.

[Vampire]

Yeah, sure, be my guest, would be great. :-)