gnodet-bot commented on code in PR #13603:
URL: https://github.com/apache/maven/pull/13603#discussion_r4304845758
##########
api/maven-api-cli/src/main/java/org/apache/maven/api/cli/mvn/MavenOptions.java:
##########
@@ -78,6 +78,26 @@ public interface MavenOptions extends Options {
@Nonnull
Optional<Boolean> suppressSnapshotUpdates();
+ /**
+ * The update policy to apply to artifacts ('always', 'daily', 'never').
Review Comment:
💡 **Missing `interval:N` from the list of accepted values.**
`RepositoryPolicy` supports four values: `always`, `daily`, `never`, and
`interval:N` (where N is minutes). The Javadoc lists only the first three, so a
user who passes `--artifacts-update-policy interval:60` will succeed at the CLI
but be confused by the undocumented syntax.
Same issue on line 92 (`metadataUpdatePolicy`).
```suggestion
* The update policy to apply to artifacts ('always', 'daily', 'never',
'interval:N').
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/aether/DefaultRepositorySystemSessionFactory.java:
##########
@@ -195,10 +195,22 @@ public SessionBuilder
newRepositorySessionBuilder(MavenExecutionRequest request)
sessionBuilder.setOffline(request.isOffline());
sessionBuilder.setChecksumPolicy(request.getGlobalChecksumPolicy());
- sessionBuilder.setUpdatePolicy(
- request.isNoSnapshotUpdates()
- ? RepositoryPolicy.UPDATE_POLICY_NEVER
- : request.isUpdateSnapshots() ?
RepositoryPolicy.UPDATE_POLICY_ALWAYS : null);
+
+ if (request.getArtifactsUpdatePolicy() == null &&
request.getMetadataUpdatePolicy() == null) {
+ // we go "old" way
+ sessionBuilder.setUpdatePolicy(
+ request.isNoSnapshotUpdates()
+ ? RepositoryPolicy.UPDATE_POLICY_NEVER
+ : request.isUpdateSnapshots() ?
RepositoryPolicy.UPDATE_POLICY_ALWAYS : null);
+ } else {
+ // we go "new" way
+ if (request.getArtifactsUpdatePolicy() != null) {
+
sessionBuilder.setArtifactUpdatePolicy(request.getArtifactsUpdatePolicy());
+ }
+ if (request.getMetadataUpdatePolicy() != null) {
+
sessionBuilder.setMetadataUpdatePolicy(request.getMetadataUpdatePolicy());
+ }
+ }
Review Comment:
⚠️ **Silent suppression of `-U` / `-nsu` in the "new way" path.**
When the user passes **only one** of the new flags — e.g.
`--metadata-update-policy daily` without `--UA` — this else-branch fires
because `metadataUpdatePolicy != null`. Inside the branch,
`isUpdateSnapshots()` and `isNoSnapshotUpdates()` are never consulted, so `-U`
and `-nsu` are silently discarded for both artifact and metadata policies.
Concrete failure scenario: `mvn -U --metadata-update-policy daily` — the
user expects `-U` to force artifact updates while setting a daily interval for
metadata. Instead, artifact update policy is left at the resolver default
(`null` → inherits repository config) and `-U` has no effect.
This needs one of:
1. Document the mutual-exclusion in the option description (new flags
supersede `-U`/`-nsu` entirely), OR
2. Honour `-U`/`-nsu` in the new-way path for the unset dimension:
```suggestion
} else {
// we go "new" way: apply legacy flags for any dimension not
explicitly set
String legacyPolicy = request.isNoSnapshotUpdates()
? RepositoryPolicy.UPDATE_POLICY_NEVER
: request.isUpdateSnapshots() ?
RepositoryPolicy.UPDATE_POLICY_ALWAYS : null;
sessionBuilder.setArtifactUpdatePolicy(
request.getArtifactsUpdatePolicy() != null ?
request.getArtifactsUpdatePolicy() : legacyPolicy);
sessionBuilder.setMetadataUpdatePolicy(
request.getMetadataUpdatePolicy() != null ?
request.getMetadataUpdatePolicy() : legacyPolicy);
}
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/CommonsCliMavenOptions.java:
##########
@@ -354,6 +381,27 @@ protected void
prepareOptions(org.apache.commons.cli.Options options) {
.longOpt("ignore-transitive-repositories")
.desc("If set, Maven will ignore remote repositories
introduced by transitive dependencies.")
.get());
+ options.addOption(Option.builder(UPDATE_METADATA)
+ .longOpt("update-metadata")
+ .desc(
+ "Forces updates of remote repository metadata
(updates plugin prefixes, version ranges and snapshots)")
+ .get());
+ options.addOption(Option.builder(UPDATE_ARTIFACTS)
+ .longOpt("update-artifacts")
+ .desc("Forces checks for missing artifacts (retries cached
retrieval errors)")
+ .get());
+ options.addOption(Option.builder()
+ .longOpt(ARTIFACTS_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto artifacts
('always', 'daily', 'never'); The -UA option overrides this option with
'always'")
Review Comment:
CLI help text also omits `interval:N`. Apply the same fix here and in
`CLIManager.java` lines 363/369.
```suggestion
"The update policy to apply onto artifacts
('always', 'daily', 'never', 'interval:N'); The -UA option overrides this
option with 'always'")
```
##########
api/maven-api-cli/src/main/java/org/apache/maven/api/cli/mvn/MavenOptions.java:
##########
@@ -78,6 +78,26 @@ public interface MavenOptions extends Options {
@Nonnull
Optional<Boolean> suppressSnapshotUpdates();
+ /**
+ * The update policy to apply to artifacts ('always', 'daily', 'never').
+ * The --UA flag overrides this with 'always'.
+ *
+ * @return an {@link Optional} containing the artifacts update policy, or
empty if not specified
+ * @since 4.0.0
+ */
+ @Nonnull
+ Optional<String> artifactsUpdatePolicy();
+
+ /**
+ * The update policy to apply to metadata ('always', 'daily', 'never').
Review Comment:
Same `interval:N` omission as line 82.
```suggestion
* The update policy to apply to metadata ('always', 'daily', 'never',
'interval:N').
```
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/CLIManager.java:
##########
@@ -339,6 +347,27 @@ public CLIManager() {
.longOpt("ignore-transitive-repositories")
.desc("If set, Maven will ignore remote repositories
introduced by transitive dependencies.")
.build());
+ options.addOption(Option.builder(UPDATE_METADATA)
+ .longOpt("update-metadata")
+ .desc(
+ "Forces updates of remote repository metadata (updates
plugin prefixes, version ranges and snapshots)")
+ .build());
+ options.addOption(Option.builder(UPDATE_ARTIFACTS)
+ .longOpt("update-artifacts")
+ .desc("Forces checks for missing artifacts (retries cached
retrieval errors)")
+ .build());
+ options.addOption(Option.builder()
+ .longOpt(ARTIFACTS_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto artifacts ('always',
'daily', 'never'); The -UA option overrides this option with 'always'")
Review Comment:
```suggestion
"The update policy to apply onto artifacts
('always', 'daily', 'never', 'interval:N'); The -UA option overrides this
option with 'always'")
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvn/CommonsCliMavenOptions.java:
##########
@@ -354,6 +381,27 @@ protected void
prepareOptions(org.apache.commons.cli.Options options) {
.longOpt("ignore-transitive-repositories")
.desc("If set, Maven will ignore remote repositories
introduced by transitive dependencies.")
.get());
+ options.addOption(Option.builder(UPDATE_METADATA)
+ .longOpt("update-metadata")
+ .desc(
+ "Forces updates of remote repository metadata
(updates plugin prefixes, version ranges and snapshots)")
+ .get());
+ options.addOption(Option.builder(UPDATE_ARTIFACTS)
+ .longOpt("update-artifacts")
+ .desc("Forces checks for missing artifacts (retries cached
retrieval errors)")
+ .get());
+ options.addOption(Option.builder()
+ .longOpt(ARTIFACTS_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto artifacts
('always', 'daily', 'never'); The -UA option overrides this option with
'always'")
+ .get());
+ options.addOption(Option.builder()
+ .longOpt(METADATA_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto metadata
('always', 'daily', 'never'); The -UM option overrides this option with
'always'")
Review Comment:
```suggestion
"The update policy to apply onto metadata
('always', 'daily', 'never', 'interval:N'); The -UM option overrides this
option with 'always'")
```
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/CLIManager.java:
##########
@@ -339,6 +347,27 @@ public CLIManager() {
.longOpt("ignore-transitive-repositories")
.desc("If set, Maven will ignore remote repositories
introduced by transitive dependencies.")
.build());
+ options.addOption(Option.builder(UPDATE_METADATA)
+ .longOpt("update-metadata")
+ .desc(
+ "Forces updates of remote repository metadata (updates
plugin prefixes, version ranges and snapshots)")
+ .build());
+ options.addOption(Option.builder(UPDATE_ARTIFACTS)
+ .longOpt("update-artifacts")
+ .desc("Forces checks for missing artifacts (retries cached
retrieval errors)")
+ .build());
+ options.addOption(Option.builder()
+ .longOpt(ARTIFACTS_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto artifacts ('always',
'daily', 'never'); The -UA option overrides this option with 'always'")
+ .build());
+ options.addOption(Option.builder()
+ .longOpt(METADATA_UPDATE_POLICY)
+ .hasArg(true)
+ .desc(
+ "The update policy to apply onto metadata ('always',
'daily', 'never'); The -UM option overrides this option with 'always'")
Review Comment:
```suggestion
"The update policy to apply onto metadata ('always',
'daily', 'never', 'interval:N'); The -UM option overrides this option with
'always'")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
