Skip to content

Conversation

Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Sep 13, 2024

Adds a health check to the host for Milvus resource so that it works with WaitFor.

Related #5645

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?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-components label Sep 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 13, 2024
@davidfowl davidfowl added the area-integrations Issues pertaining to Aspire Integrations packages label Sep 13, 2024
@davidfowl
Copy link
Member

Looks like the health check isn't working

}
internal static MilvusClient CreateMilvusClient(IServiceProvider sp, string? connectionString)
{
if (connectionString is null)
Copy link
Member

@davidfowl davidfowl Sep 16, 2024

Choose a reason for hiding this comment

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

Ideally we would share this logic:

public sealed class MilvusClientSettings

@eerhardt thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @davidfowl we should do this because the created Milvus connection string in app-host isn't compatible with MilvusClient and those properties (database, endpoint, key) should be extracted before creating MilvusClient.

Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason this wasn't done?


var milvus = new MilvusServerResource(name, apiKeyParameter);

string? connectionString = null;
Copy link
Member

Choose a reason for hiding this comment

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

Probably technically don't need the connection string here since you have the client.

@mitchdenny mitchdenny merged commit 8fd65d2 into dotnet:main Sep 17, 2024
11 checks passed
@Alirexaa Alirexaa deleted the waitfor-milvus branch September 17, 2024 14:01
@github-actions github-actions bot locked and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-integrations Issues pertaining to Aspire Integrations packages 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.

4 participants