Skip to content

Conversation

BrennanConroy
Copy link
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
Contributor

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

@davidfowl davidfowl requested a review from mitchdenny November 30, 2023 05:18
Copy link
Member

@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.

@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
Member

@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](dotnet#177) and [PR 1121](dotnet#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 dotnet#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 <[email protected]>
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 <[email protected]>
Co-authored-by: Mitch Denny <[email protected]>
@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