Skip to content

Add support for passing arguments to containers#1121

Merged
BrennanConroy merged 3 commits intomainfrom
brecon/containerarg
Nov 30, 2023
Merged

Add support for passing arguments to containers#1121
BrennanConroy merged 3 commits intomainfrom
brecon/containerarg

Conversation

@BrennanConroy
Copy link
Copy Markdown
Member

Closes #177

Questions:

  • Should we have a separate annotation type for container args instead of reusing executable args?
  • Args need to be separated into different list items. Should we make it params string[]? I'm worried about arbitrarily splitting the args by space in case something like 'Program Files' needs to be in a single list entry.
  • Manifest generation is very hardcoded right now so it doesn't automatically support args in containers. Should it be more flexible i.e. it knows how to write specific annotations as well as specific resources? So we don't need to add args to WriteContainer

@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Nov 29, 2023
@smitpatel
Copy link
Copy Markdown
Contributor

Filed #1122 for follow-up for dashboard to light up the information.

Comment thread src/Aspire.Hosting/Extensions/ContainerResourceBuilderExtensions.cs Outdated
@davidfowl davidfowl requested a review from mitchdenny November 30, 2023 05:18
Copy link
Copy Markdown
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

Thanks Brennan. I think we need a couple of changes, but overall looks good.

Comment thread src/Aspire.Hosting/Extensions/ContainerResourceBuilderExtensions.cs Outdated
Comment thread src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated
@BrennanConroy BrennanConroy marked this pull request as ready for review November 30, 2023 19:58
@BrennanConroy BrennanConroy added this to the preview 2 (Dec) milestone Nov 30, 2023
@BrennanConroy BrennanConroy merged commit 93dbd75 into main Nov 30, 2023
@BrennanConroy BrennanConroy deleted the brecon/containerarg branch November 30, 2023 23:27
andrevlins pushed a commit to andrevlins/aspire that referenced this pull request Dec 3, 2023
@davidfowl
Copy link
Copy Markdown
Contributor

@BrennanConroy we should also consider making this a callback so that we can get allocated endpoints from other resources.

ericmutta added a commit to ericmutta/aspire that referenced this pull request Dec 8, 2023
The wording in the XML docs is based on my cursory review of [issue 177](microsoft#177) and [PR 1121](microsoft#1121).

I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley).

#Fixes microsoft#1294
mitchdenny added a commit that referenced this pull request Dec 12, 2023
* Add XML Docs for WithArgs()

The wording in the XML docs is based on my cursory review of [issue 177](#177) and [PR 1121](#1121).

I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley).

#Fixes #1294

* Update ContainerResourceBuilderExtensions.cs

---------

Co-authored-by: Mitch Denny <midenn@microsoft.com>
github-actions Bot pushed a commit that referenced this pull request Dec 12, 2023
The wording in the XML docs is based on my cursory review of [issue 177](#177) and [PR 1121](#1121).

I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley).

#Fixes #1294
DamianEdwards pushed a commit that referenced this pull request Dec 13, 2023
* Add XML Docs for WithArgs()

The wording in the XML docs is based on my cursory review of [issue 177](#177) and [PR 1121](#1121).

I should add that I only started reviewing Aspire a few hours ago, so apologies if the XML docs aren't accurate - I just figured the best way to get started contributing is to jump right in (CC: @danmoseley).

#Fixes #1294

* Update ContainerResourceBuilderExtensions.cs

---------

Co-authored-by: Eric Mutta <eric.mutta@gmail.com>
Co-authored-by: Mitch Denny <midenn@microsoft.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 28, 2024
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support specifying the args for running a container

4 participants