-
Notifications
You must be signed in to change notification settings - Fork 592
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
Versionless Repo Resolver additions and Error handling #29058
Versionless Repo Resolver additions and Error handling #29058
Conversation
31282d3
to
0b4724f
Compare
#build |
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/_D76ycEDsEe-Hn95lLM_tGQ Target locations of links might be accessible only to IBM employees. |
Your personal pipeline request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=cfc87762-a5c7-488a-b3ad-c363329f95b1 Target locations of links might be accessible only to IBM employees. |
#build |
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_uvx5gEFnEe-F88sHKWQ5jw Target locations of links might be accessible only to IBM employees. |
Your personal pipeline request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=16e920e3-68e5-4d09-bd64-db62938010fa Target locations of links might be accessible only to IBM employees. |
The build cbridgha-29058-20240713-0155 For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_D76ycEDsEe-Hn95lLM_tGQ |
The build cbridgha-29058-20240713-1640 For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_uvx5gEFnEe-F88sHKWQ5jw |
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
...epository.resolver/src/com/ibm/ws/repository/resolver/internal/kernel/KernelResolverEsa.java
Outdated
Show resolved
Hide resolved
for (FeatureResource versionedFeature : feature.getConstituents(SubsystemContentType.FEATURE_TYPE)) { | ||
//Find the right public feature (should only be one) - set the result | ||
ProvisioningFeatureDefinition versionedFeatureDef = getFeature(versionedFeature.getSymbolicName()); | ||
if (versionedFeatureDef.getVisibility() != Visibility.PUBLIC) { |
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.
If there is only one result expected we will still keep iterating over the list, worth considering reversing the check to
if (versionedFeatureDef.getVisibility() == Visibility.PUBLIC) {
return versionedFeatureDef;
// OR
result = versionedFeatureDef;
break;
}
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.
thanks will do
#build |
if (versionedFeatureDef.getVisibility() == Visibility.PUBLIC) { | ||
return versionedFeatureDef; | ||
} | ||
result = versionedFeatureDef; |
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.
not sure on the logic here, if a public feature is found return that feature, but if one isn't it still sets the result to the feature, I am assuming that this line (and the break) are not needed or it will just look at the first feature and return it regardless of whether it is public
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.
no longer create a "result" variable - makes no sense... - added the possiblibilty of returning null in javadoc
break; | ||
} | ||
} | ||
return result; |
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.
If we get to this point then we have not found a public feature, is this an error case?
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.
not an error but return null
Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_ZBYEgEL1Ee-F88sHKWQ5jw Target locations of links might be accessible only to IBM employees. |
Your personal pipeline request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=dffbf0e8-1c24-4520-ba1d-58e2ff30dcda Target locations of links might be accessible only to IBM employees. |
88eb15a
to
c8e60d8
Compare
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
...epository.resolver/src/com/ibm/ws/repository/resolver/internal/kernel/KernelResolverEsa.java
Outdated
Show resolved
Hide resolved
* @return ProvisioningFeatureDefinition | ||
*/ | ||
private ProvisioningFeatureDefinition getVersionedFeature(String versionlessLinkingFeatureName) { | ||
ProvisioningFeatureDefinition result = null; |
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.
result is never used is it (now the method returns null at the end)?
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.
removed result
ProvisioningFeatureDefinition feature = resolverRepository.getFeature(name); | ||
if (feature != null && feature.isVersionless()) { | ||
ProvisioningFeatureDefinition firstChild = resolverRepository.findAllPossibleVersions(feature).get(0); | ||
String plat = firstChild.getPlatformName(); |
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.
do we need to null check firstChild?
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.
If we do, we need to check whether the list is empty before calling get(0)
because that will throw an exception.
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.
Agreed made changes to check
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
/** | ||
* @return the resolvedPlatforms | ||
*/ | ||
public Set<String> getResolvedPlatforms() { | ||
return resolvedPlatforms; | ||
} | ||
|
||
/** | ||
* @return the missingPlatforms | ||
*/ | ||
public Set<String> getMissingPlatforms() { | ||
return missingPlatforms; | ||
} | ||
|
||
/** | ||
* @return the missingBasePlatforms | ||
*/ | ||
public List<String> getMissingBasePlatforms() { | ||
return missingBasePlatforms; | ||
} |
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.
Please add more documentation about what each of these fields means and what data it contains.
These fields look like they are used in other places, but here seems like the most logical place to document them.
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.
There's a little bit of doc on the constructor and it sounds like missingPlaforms
and missingBasePlatforms
are completely different? That could do with a better explanation and possibly missingBasePlatforms
should be renamed.
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.
yes tried to add javadoc - one is simply a missspelling of a platform name - one is related to a base name like MP or jakarta realted to a versionless feature - if they can't be derived up front - its an error, and the reason the feature isn't resolved.
/** | ||
* List of all the missing platforms after resolution | ||
*/ | ||
Set<String> missingPlatforms; |
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.
What does it mean for a platform to be "missing"?
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.
Will a platform name only be in this set if the user requested it explicitly?
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
dev/com.ibm.ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolver.java
Outdated
Show resolved
Hide resolved
...ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolutionException.java
Outdated
Show resolved
Hide resolved
...ws.repository.resolver/src/com/ibm/ws/repository/resolver/RepositoryResolutionException.java
Show resolved
Hide resolved
6571a97
to
65018d4
Compare
The build cbridgha-29058-20240715-1755 |
65018d4
to
9576b50
Compare
The build cbridgha-29058-20240716-1034 For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_Kal94EOQEe-F88sHKWQ5jw |
This PR uses the resolution Result object to start gathering additional results that could occur in versionless scenarios.
Primarily two main scenarios where messages have been added to the install commands (#29060). Platform can't be found, or platform can't be determined from the set of versionless/versioned features passed.
A few additions to the Repository to handle querying the platform header info now stored in the ESA's.
Started adding install tests, but will be exploring how to test for features not yet part of the public repo.