Skip to content

Change PublisherOptions - Allow skipping image building?#8563

Merged
davidfowl merged 1 commit intomicrosoft:mainfrom
prom3theu5:feature/control-image-building
Apr 15, 2025
Merged

Change PublisherOptions - Allow skipping image building?#8563
davidfowl merged 1 commit intomicrosoft:mainfrom
prom3theu5:feature/control-image-building

Conversation

@prom3theu5
Copy link
Copy Markdown
Contributor

@prom3theu5 prom3theu5 commented Apr 4, 2025

Update property access to use PublisherOptions directly for consistency. Introduce a new option to control image building during the publishing process. Ensure that image building only occurs when enabled. Adjust tests to accommodate these changes.

Just thinking a user probably wouldn't want to rebuild and re-version all project container images if they were simply changing and publishing a new compose for container resource changes (new env vars, new tags etc).

Fixes #8782

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

Update property access to use `PublisherOptions` directly for 
consistency. Introduce a new option to control image building during 
the publishing process. Ensure that image building only occurs when 
enabled. Adjust tests to accommodate these changes.
Copilot AI review requested due to automatic review settings April 4, 2025 21:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs:120

  • Consider adding a unit test to verify that the .env file is created and populated with the expected environment variables.
var envFile = Path.Combine(PublisherOptions.OutputPath!, ".env");

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 4, 2025
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 4, 2025
/// <summary>
/// Indicates whether to build container images during the publishing process.
/// </summary>
public bool BuildImages { get; set; } = true;
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.

Should negate this bool here?

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.

Have it default false and explicit opt in to builds?

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.

I'm not sure we have a way to flow command line arguments from the publisher, so this needs to be configured in code (I believe).

@mitchdenny ?

Copy link
Copy Markdown
Contributor Author

@prom3theu5 prom3theu5 Apr 5, 2025

Choose a reason for hiding this comment

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

A clean win without polluting cli could be to handle extra publisher options from IConfiguration and the AppHosts AppSettings?
Then just utilise the configuration binder Get extension to populate an instance of "Optional" config, and "Merge" that onto the incoming (from cli) Publisher Options?
Maybe a small source generator to generate a MergeExtension, for each PublisherSettings derived instance?

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.

To be fair, could just be manually assigned, without the MergeExtension - we're only talking about a handful of options for each publisher - The helm one has more that Compose, and will potentially require even more for extension

I guess another option would be a new ManifestExpression, and have config directly in the AppHost declaratively

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We didn't get to mapping switches to config in 9.2 but as you've alluded the plan is to make use of IConfiguration. When registering publishers I'll probably have an optional mapping mechanism from switches to config sections.

@danmoseley danmoseley added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 7, 2025
@davidfowl davidfowl merged commit 76b6f32 into microsoft:main Apr 15, 2025
175 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators May 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aspire publish --publisher docker-compose should accept a parameter to not build images

5 participants