SED-4713 isolated-automation-package-repository-canonical-plan-names-are-not-canonical#657
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces CANONICAL_REPOSITORY_PARAMETER_KEYS sets in both IsolatedAutomationPackageRepository and MavenArtifactRepository to define canonical repository parameters. However, the feedback highlights that both sets are missing critical parameters—such as PARAM_WRAP_PLANS_INTO_TEST_SET, PARAM_ROOT_TYPE, ARTIFACT_PARAM_CLASSIFIER, and ARTIFACT_PARAM_TYPE—which could lead to incorrect plan caching or resolution issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); |
There was a problem hiding this comment.
The CANONICAL_REPOSITORY_PARAMETER_KEYS set is missing PARAM_WRAP_PLANS_INTO_TEST_SET and PARAM_ROOT_TYPE. Since these parameters directly affect the structure, name, and type of the imported plan (as seen in getArtefactInfo and isWrapPlansIntoTestSet), omitting them can lead to incorrect plan caching or resolution when these parameters differ between executions.
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); | |
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_WRAP_PLANS_INTO_TEST_SET, | |
| ArtifactRepositoryConstants.PARAM_ROOT_TYPE); |
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID, | ||
| ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); |
There was a problem hiding this comment.
The CANONICAL_REPOSITORY_PARAMETER_KEYS set is missing PARAM_WRAP_PLANS_INTO_TEST_SET, ARTIFACT_PARAM_CLASSIFIER, and ARTIFACT_PARAM_TYPE. Since classifier and type uniquely identify the resolved Maven artifact, and wrapPlans determines the structure of the imported plan, omitting them can cause different artifacts or plan structures to share the same canonical parameters, leading to incorrect plan caching or resolution.
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID, | |
| ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); | |
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(ArtifactRepositoryConstants.ARTIFACT_PARAM_GROUP_ID, | |
| ArtifactRepositoryConstants.ARTIFACT_PARAM_ARTIFACT_ID, ArtifactRepositoryConstants.ARTIFACT_PARAM_VERSION, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | |
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES, | |
| ArtifactRepositoryConstants.PARAM_WRAP_PLANS_INTO_TEST_SET, | |
| ArtifactRepositoryConstants.ARTIFACT_PARAM_CLASSIFIER, | |
| ArtifactRepositoryConstants.ARTIFACT_PARAM_TYPE); |
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); |
There was a problem hiding this comment.
It would feel more consistent to remove the version and keep only groupId + artifactId
| public static final Set<String> CANONICAL_REPOSITORY_PARAMETER_KEYS = Set.of(RepositoryWithAutomationPackageSupport.AP_NAME, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_PLANS, | ||
| ArtifactRepositoryConstants.PARAM_INCLUDE_CATEGORIES, | ||
| ArtifactRepositoryConstants.PARAM_EXCLUDE_CATEGORIES); |
… not canonical