Skip to content

Improve ContainerSpecHelper partial image configuration error message#4807

Merged
dimas-b merged 3 commits into
apache:mainfrom
venkateshwaracholan:issue-4806-container-spec-helper-error-message
Jun 18, 2026
Merged

Improve ContainerSpecHelper partial image configuration error message#4807
dimas-b merged 3 commits into
apache:mainfrom
venkateshwaracholan:issue-4806-container-spec-helper-error-message

Conversation

@venkateshwaracholan

Copy link
Copy Markdown
Contributor

Summary

Fixes #4806

Improve the error message shown when only one of the container image name or image tag is configured.

Changes

  • Reword the IllegalArgumentException message to improve clarity.
  • Remove awkward wording and the double-space in the message.
  • Clarify that both image name and image tag must be specified together, or both omitted to use the default image configuration.
  • Add tests covering valid and invalid image configuration combinations.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@dimas-b dimas-b left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new message change looks better 🙂 Thanks, @venkateshwaracholan !

@Test
void dockerImageNameUsesBothSystemPropertiesWhenProvided() {
System.setProperty("polaris.testing." + CONTAINER_NAME + ".image", "docker.io/custom/image");
System.setProperty("polaris.testing." + CONTAINER_NAME + ".tag", "9.9.9");

@dimas-b dimas-b Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, using System properties during unit tests is not nice and prone to inter-test side effects.

I support the idea of adding test coverage to the image selection logic, though. I'd propose to add a test-only variant for containerSpecHelper() that would take Function<String, String> for both the sys. prop. and env. lookups (overridden by tests). The "prod" code will use trivial redirects on the main call path.

This way we'd be able to validate env. lookup logic too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dimas-b Thanks for the suggestion. I updated the tests to inject property and environment lookups instead of modifying JVM system properties directly. The production path is unchanged, and the new approach also allowed adding coverage for environment variable handling and precedence rules.

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 18, 2026
@dimas-b dimas-b merged commit 5d08f04 into apache:main Jun 18, 2026
24 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve ContainerSpecHelper partial image configuration error message

2 participants