-
Notifications
You must be signed in to change notification settings - Fork 591
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
Add new edtion and product id for Open Liberty #3215
Conversation
Code analysis and actionsDO NOT DELETE THIS COMMENT.
|
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_W7wHkURyEei6Isn3iLaRtw Target locations of links might be accessible only to IBM employees. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the comments are blockers.
* Open liberty features with an edition of OPEN in the appliesTo can be | ||
* installed on everything except CORE. | ||
*/ | ||
private final static List<String> nonCoreEditions = Arrays.asList("Base", "Express", "Developers", "ND", "z/OS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we use these constants in more than one place, I'd be a bit tempted to move them out into named constants (or enums)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense
for (List<String> parsedProduct : parsedProducts) { | ||
AppliesToEntry entry = new AppliesToEntry(); | ||
for (String thing : parsedProduct) { | ||
entry.add(thing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thing
isn't a great name
productChunk
as used above might be better.
Also, it's not obvious reading the code what the difference between an AppliesToEntry
and an AppliesToFilterInfo
is. I assume we need methods for both though?
* into a list of lists, so something like | ||
* <blah,blah,blah>,<foo,foo,foo>,<baz,baz,baz> | ||
*/ | ||
private static List<List<String>> doParse(String appliesTo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comment 👍
AppliesToProcessor needs to recognize OPEN as a valid edition and also have an appropriate mapping to a human readable edition. OPEN features can be installed on any edition apart from CORE Also do a bit of refactoring to put the parsing logic into a common method, and make a couple of methods private, as they clearly should have been
#build |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_qFx2MESjEei6Isn3iLaRtw Target locations of links might be accessible only to IBM employees. |
The build idlewis-3215-20180420-0917 |
The build idlewis-3215-20180420-1511 |
#build |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_mGwIAEZoEeiBcaHL18STjw Target locations of links might be accessible only to IBM employees. |
The build idlewis-3215-20180422-2112 |
The most recent build is red due to one UI test failure, which is not related to this change |
To distinguish features from WebSphere Liberty features
#build