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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b81e688
JBDS-3187 remove custom rules since they cause cyclic dependency and …
maxandersen Oct 19, 2014
4fd075f
JBDS-3187 Check BUILD_ALIAS with standard enforcer rule
maxandersen Oct 19, 2014
74838c2
JBDS-3187 use regex that fails GAa and CR for BUILD_ALIAS.
maxandersen Oct 23, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 16 additions & 16 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
<name>JBDS - Product, Installers</name>
<packaging>pom</packaging>
<modules>
<module>rules</module>
<module>plugins</module>
<module>features</module>
<module>site</module>
Expand Down Expand Up @@ -51,23 +50,24 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
<version>1.0-beta-1</version>
<dependencies>
<dependency>
<groupId>com.jboss.devstudio</groupId>
<artifactId>ParentPomVersionCheckRule</artifactId>
<version>0.1</version>
</dependency>
</dependencies>
<version>1.3.1</version>
<executions>
<execution>
<id>enforce</id>
<configuration><rules>
<ParentPomVersionCheckRule implementation="com.jboss.devstudio.enforcer.rule.ParentPomVersionCheckRule"><shouldIfail>true</shouldIfail></ParentPomVersionCheckRule>
</rules></configuration>
<goals>
<goal>enforce</goal>
</goals>
<id>enforce-properversion</id>
<goals>
<goal>enforce</goal>
</goals>
<configuration>
<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.

<regex>.*(Alpha|Beta|CR|GA)\d?[a-z]?</regex>
Copy link
Member

Choose a reason for hiding this comment

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

This regex will pass for Alpha, Beta, CR (which are invalid) as well as Alpha1, Beta2, CR3 (which are valid).

Maybe you meant to have something more like ((Alpha|Beta|CR)\d+[a-z]?)|GA)

Copy link
Member Author

Choose a reason for hiding this comment

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

True, thats what I referred to in the initial comment that it did pass some other corner cases.

Your regular expression definitely is better - I'll try that for sure.

Copy link
Member

Choose a reason for hiding this comment

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

Winning at collaboration. :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the PR to use the more complete regex suggested.

And yes, collaboration++ and simple commit history++ :)

<regexMessage>The BUILD_ALIAS (${BUILD_ALIAS}) is invalid. Must contain either Alpha, Beta, CR or GA + optional postfix.</regexMessage>
</requireProperty>
</rules>
<fail>true</fail>
</configuration>
</execution>
</executions>
</plugin>
Expand Down
73 changes: 0 additions & 73 deletions rules/com.jboss.devstudio.enforcer.rule/pom.xml
View file
Open in desktop

This file was deleted.

124 changes: 0 additions & 124 deletions ...orcer.rule/src/main/java/com/jboss/devstudio/enforcer/rule/ParentPomVersionCheckRule.java
View file
Open in desktop

This file was deleted.

152 changes: 0 additions & 152 deletions rules/com.jboss.devstudio.enforcer.rule/usage-pom.xml
View file
Open in desktop

This file was deleted.