Skip to content

refactor(tasks): ArtifactTask reads its configuration from the task args - #2308

Open
pirhoo wants to merge 9 commits into
mainfrom
fix/2298-artifact-task-args
Open

refactor(tasks): ArtifactTask reads its configuration from the task args#2308
pirhoo wants to merge 9 commits into
mainfrom
fix/2298-artifact-task-args

Conversation

@pirhoo

@pirhoo pirhoo commented Aug 3, 2026

Copy link
Copy Markdown
Member

Closes #2298.

ARTIFACT was the only pipeline stage reading process-wide properties instead of its task args. Moving it to task args exposed four bugs in how a -s settings file resolves against CLI option defaults, on both the legacy jopt parser and the picocli one, so those are fixed here too.

  • refactor: ArtifactTask reads its configuration from the task args instead of the injected process-wide PropertiesProvider, like every other stage already does
  • fix: substitute a settings-file value before skipping an option that declares no default, so a settings-only artifactDir reaches the ARTIFACT stage instead of failing it
  • fix: consult the settings file only when the operator passed -s, so a classpath datashare.properties no longer outranks every option default
  • fix: exclude arity-0 booleans from the picocli default value provider, so a settings file setting resume=true no longer makes -r mean "do not resume"
  • fix: read the settings file alone via the new PropertiesProvider.getFileProperties(), so DS_DOCKER_* env vars keep the precedence CommonMode gives them instead of beating an option default only when -s happens to be passed
  • test: cover the precedence bugs in DatashareCliTest, SettingsDefaultsTest and PropertiesProviderTest

Not covered

  • -s<path> (attached short form) is still not parsed by settingsPathFrom, so the settings file is ignored on the picocli path for that invocation style.
  • ArtifactTask reads task args through java.util.Properties, so a non-String arg from the API or Temporal is silently dropped.
  • DS_DOCKER_* env vars still lose to any option that declares a default, because CommonMode layers the CLI properties on top. That contradicts the precedence documented on SettingsResource.patchSettings, but it predates this branch and changing it would affect every Docker deployment, so it is left alone here.

@pirhoo pirhoo self-assigned this Aug 3, 2026
@pirhoo pirhoo added this to Sprint 49 Aug 3, 2026
@pirhoo pirhoo moved this to In Review in Sprint 49 Aug 3, 2026
@pirhoo
pirhoo force-pushed the fix/2298-artifact-task-args branch from aa1c236 to 2a80136 Compare August 10, 2026 15:47
@pirhoo
pirhoo requested a review from a team August 10, 2026 17:02
@pirhoo
pirhoo marked this pull request as ready for review August 10, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

refactor: ARTIFACT is the only pipeline stage reading process-wide properties instead of its task args

1 participant