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

JBDS-3187 Check BUILD_ALIAS with standard property enforcer rule. #237

Closed
wants to merge 3 commits into from

Conversation

maxandersen
Copy link
Member

Also removed custom rules and custom module - if we get more complex rules one day we can
add it back, but should be external since otherwise the parent pom and the rules will have a cyclic dependency.

Tested by using:
mvn -Dtycho.mode=maven verify -DBUILD_ALIAS=

CR1 (pass)
CR1a (pass)
(fail - must be specified)
Final (fail - Final is not allowed)

Two cases are allowed in even though properly not proper:
CR (passes, but should fail since it needs a number)
GA1a (passes, but there should not be multiple GA's nor respins)

But this at least catches the most important one - not having Final in the name.

@maxandersen
Copy link
Member Author

Mind you that most changes in here is to revert the 3(?) PR's regarding JBDS-3187.

End result is the diff is very small - just a section in pom.xml that defines the property check.

…does not provide value over a simple property check
@maxandersen
Copy link
Member Author

pushed update to separate the changes into two commits allowing to cherry pick the actual change to a branch.

Also bumped version of enforcer to use more recent version of enforcer.

Tested by using:
mvn -Dtycho.mode=maven verify -DBUILD_ALIAS=

CR1 (pass)
CR1a (pass)
(fail - must be specified)
Final (fail - Final is not allowed)

Two cases are allowed in even though properly not proper:
CR (passes, but should fail since it needs a number)
GA1a (passes, but there should not be multiple GA's nor respins)

But this at least catches the most important one - not having Final in the name.
<rules>
<requireProperty>
<property>BUILD_ALIAS</property>
<message>BUILD_ALIAS is missing, it must be specificed.</message>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said you wanted these rules implemented somewhere other than in the jbdevstudio-product repo?

This is certainly simpler than my custom rule, but if in future we want to do other pre-build checks we won't be able to do so if they're more complex than a property-matches-regex check.

This message needs to be rewritten, for two reasons. Since jbdevstudio-product/pom.xml inherits BUILD_ALIAS from the jbosstools-build/parent/pom.xml file, which is derived from the jbosstools-build/parent/pom.xml version, this value will ALWAYS be set.

An alternative simpler and more accurate message (taken from your ) could be:

"BUILD_ALIAS must contain either Alpha, Beta, CR or GA + optional postfix."

Note also that "specificed" is not a word. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said the custom code for the enforcer rule should be somewhere else since otherwise the dependencies are cyclic.

In my PR I'm not doing a custom rule, just using it - just like your PR used your custom rule.

I don't understand what you mean it will always be set - the whole point is to catch if we change something that causes it to be wrong or unset. Thus that needs checking.

I'll fix the other minor wording ones.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUILD_ALIAS is set in the parent pom. So unless JBDS stops using the parent pom, the variable will be inherited and therefore always be set.

https://github.com/jbosstools/jbosstools-build/blob/master/parent/pom.xml#L18

Also, I didn't experience any cyclic dependency but I agree your solution (for this specific individual rule) is way simpler. My point was that we might in future want more complex rules and having an existing framework in place in a rules/ folder would allow anyone to contribute new restrictions w/o them cluttering up the root pom.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And build_alias might not be set in the parent pom by accident - this validation is trying to detect failure to follow both human or mechanical error.

once we have that need/problem we can always enable it again but lets not do that before we actually have that need.

I can't think of any rules right now that would require custom code and those I can imagine would be better put outside this repository and shared with other projects.

And no, you don't experience the cyclic dependency because you probably build the child module first or because maven tries to not get in the way when you have not set the exact dependency versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the specified misspelling.

Kept the message about property not being specificed since it will only fail with that message if the property is missing.

@maxandersen
Copy link
Member Author

Can I get a +1 ?

I'll squash and apply to the branch too so we catch it on both branches.

@nickboldt
Copy link
Member

+1. Feel free to push to 4.2.x branch too, as we'll need to verify this condition doesn't happen for future 4.2.x builds too.

@maxandersen
Copy link
Member Author

This looks like it is already merged. closing.

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

Successfully merging this pull request may close these issues.

None yet

2 participants