JBDS-3187 more refactoring and tweaks >> master

[nickboldt]

No description provided.

[maxandersen]

Why does this need to be in separate module?

[maxandersen]

Okey. Now I see its to make a custom rule.

A) that really should be somewhere outside this repo Imo.
B) could this not be done wit standard banned dependencies enforcer rule?
C) if not, just use banshee avoiding all these boiler plate code?

[nickboldt]

A) Your constructive feedback is appreciated. Please elaborate on why a check done when building the devstudio-product should be housed outside the devstudio-product repo. By co-locating it, the check will fail for LOCAL builds as well as those in Jenkins. By locating it elsewhere, it won't. Rather than voicing a difference of opinion, please explain why your approach is better.

B) Perhaps, but we can also expand this single usecase to include MORE tests/validations. Don't you want to verify MORE than just one thing? Surely we can make the build BETTER with more rules and checks? This is just an initial attempt to prove it can be done, and implement it, too. Once we have a template for how to write rules, we can then write more.

C) What is Banshee and how would it apply here? Could you provide a link, sample code, etc? I tried googling for "maven banshee" and didn't find anything useful.

[maxandersen]

A) IMO you are writing a maven enforcer rule that is not devstudio specific, but sounds like you want these rules to be custom of devstudio. I prefer a few lines of xml rather than alot of boilerplate code.
B) I dont understand why we need to add custom code like this when there seem to perfectly fine standard enforcer rules for this. The day we actually have custom rules that needs custom code then i'm all for doing that.
C) damn autocorrect. was meant to write beanshell - but that was before I fully grokked that you don't actually need any custom logic here. So that point is mute since we can just remove it all.

[maxandersen]

A) btw. I do not understand why you think moving the rule implementation out to a separate repo (i.e. where our other maven rules are defined) would not make the rule fail both locally and in hudson.). This pom will still have the actual rule defined, so it will still fail.

[maxandersen]

Afaics we could even just use enforce property rule. http://maven.apache.org/enforcer/enforcer-rules/requireProperty.html against the property matching a specific pattern.

Btw. In future much better to squash the commits so let's long log.

[nickboldt]

Enforcing a value of a property is nice but it's not extensible the same way a custom rule is. And my way allows us to check for two properties (parent pom version and BUILD_ALIAS).

I considered squashing the commits but felt that having one commit that showed the initial code verbatim with the ASL license on it as copied from maven.apache.org, followed by my edits, was a better way to preserve the history of the code.

YMMV, and IANAL, but I believe my approach is better.

Your opinion matters to me, but you have not expressed a technical reason for squashing commits, and as there IS a technical/legal reason for NOT squashing them, I believe my rationale trumps your opinion.

Or is there a technical reason why squashed commits in a PR is "much better", other than your personal preference?

[maxandersen]

(technical and social) Reasons for squashing commits,
Easier to read history - one commit = one feature
Easier to merge to other branch.
Easier to revert.
Easier to understand for others than the person that wrote the commits.
git bisect works much better too.
If you want to insist on using non-squashed commits for changes like this then consider learning to use git merge --squash so there is at least a merge commit present.
More on this topic: https://coderwall.com/p/ny1hia

initial code verbatim with the ASL license on it as copied from maven.apache.org would be just as visible in the squashed commit (unless you followed up with something deleting the license which would be wrong)

[maxandersen]

Looking at this more - I don't grok why this is ever wanting to look at the parent pom version.
The parent pom version is irrelevant in this case. It is the version of the actual building component that can't have final in it.

[maxandersen]

Another reason for this rules not being inside the repository that defines it is that it won't actually be able to run the build of rules to get the latest rule afaics. The root/parent pom is using a rule define by one of its submodules which in it self is dependent on the parent pom.

I'm writing a PR to show how I think this should be done since while I think it is cool we can now write custom rules, it is not appropriate to host it in here and not (yet) necessary (if ever) to have such much code for something which usecases can be done in a few lines.

p.s. i'm just waiting for the internet to download to be able to verify my PR :)

[maxandersen]

That PR is here #237
overall result is the same (verify Final is not used as build alias), but less code, less things to mantain and no cyclic dependency from rules to parent to rules in the pom.xml.