Conversation
36e4703 to
ab59e2a
Compare
ab59e2a to
64b1917
Compare
Yes, |
a009283 to
112736d
Compare
That was done yesterday, so I'd expect things to look correct now. |
b50c898 to
e4b32b6
Compare
6429680 to
8d86d5b
Compare
1e8fa0b to
f2c4a2c
Compare
49666f5 to
c97ed34
Compare
e7648e6 to
9c42e4e
Compare
| public static bool IsEmulator(this ApplicationModel.IResource resource) { throw null; } | ||
| } | ||
|
|
||
| public static partial class EventingExtensions |
There was a problem hiding this comment.
Should we call this DistributedApplicationEventingExtensions for consistency? /cc @davidfowl
| { | ||
| public static ApplicationModel.IResourceBuilder<ExternalServiceResource> AddExternalService(this IDistributedApplicationBuilder builder, string name, ApplicationModel.IResourceBuilder<ApplicationModel.ParameterResource> urlParameter) { throw null; } | ||
|
|
||
| public static ApplicationModel.IResourceBuilder<ExternalServiceResource> AddExternalService(this IDistributedApplicationBuilder builder, string name, string url) { throw null; } |
There was a problem hiding this comment.
nit: should this be string uri. I know that we have Uri uri, but there is no reason that the string has to be a URL :)
There was a problem hiding this comment.
Unless it is very specifically HTTP/HTTPS only in which case should the API be called AddExternalHttpService(...)? /cc @DamianEdwards
There was a problem hiding this comment.
It does, we validate it.
|
|
||
| [System.Diagnostics.CodeAnalysis.Experimental("ASPIREPUBLISHERS001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public partial interface IPublishingActivityProgressReporter | ||
| public partial interface IPublishingActivityReporter |
There was a problem hiding this comment.
I think a better name would be IPipelineActivityReporter, as this isn't specific to "publishing" and it's already used in a "deploying" context as well. Ideally, pipeline could be used here as its agnostic to both publishing and deploying.
There was a problem hiding this comment.
I think just IActivityReporter.
There was a problem hiding this comment.
Pipeline doesn't mean anything yet. IOperationActivityReporter 😄
There was a problem hiding this comment.
Pipeline doesn't mean anything yet.
Sure it does! It's the line we draw between a user on the CLI calling publish or deploy and the output on their screen. 😝 That's where this is used.
IOperationActivityReporter
Too generic.
There was a problem hiding this comment.
Pipeline doesn’t mean much
There was a problem hiding this comment.
I think Operation makes sense - because we have publish and run operations - and we'll probably want to add prompting support for run as well.
There was a problem hiding this comment.
I suppose that if we make it clear that Operation represents Steps and child Tasks—which are distinct to non-run ops.
| public partial interface IPublishingActivityProgressReporter | ||
| public partial interface IPublishingActivityReporter | ||
| { | ||
| System.Threading.Tasks.Task CompletePublishAsync(string? completionMessage = null, CompletionState? completionState = null, bool isDeploy = false, System.Threading.CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Instead, consider CompletePipelineAsync as this API is used in deploying as well.
There was a problem hiding this comment.
Should just be CompleteAsync
There was a problem hiding this comment.
Don't feel like it;s for 9.4, this API is experimental
There was a problem hiding this comment.
I think renaming it to CompleteAsync makes the isDeploy parameter even more awkward.
Can the IPublishingActivityReporter know ambiently if it is a deploy operation or not?
There was a problem hiding this comment.
IMO, CompleteAsync is too vague considering there are 3 distinct concepts that can be completed here (the publish/deploy itself, steps, and tasks).
Can the IPublishingActivityReporter know ambiently if it is a deploy operation or not?
Nope, we'd have to modify this API to take PublishingOptions as input if we wanted to determine this without having an isDeploy argument.
There was a problem hiding this comment.
There should be no isDeploy parameter. It should jsut take a message.
| } | ||
|
|
||
| [System.Diagnostics.CodeAnalysis.Experimental("ASPIREPUBLISHERS001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public partial interface IPublishingStep : System.IAsyncDisposable |
There was a problem hiding this comment.
Consider renaming this to IPipelineStep, as it's used in both publish and deploy.
| } | ||
|
|
||
| [System.Diagnostics.CodeAnalysis.Experimental("ASPIREPUBLISHERS001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public partial interface IPublishingTask : System.IAsyncDisposable |
There was a problem hiding this comment.
Consider renaming this to IPipelineTask, as it's used in both publish and deploy.
|
@joperezr can we merge this |
|
On it, just need to rerun real quick |
|
Merged with end state as of release 9.4.0. Thanks everyone! |
Auto-generated update to the API surface to compare current surface vs latest release. This should only be merged once this surface area ships in a new release.