Skip to content

Conversation

@timheuer
Copy link
Member

@timheuer timheuer commented Aug 9, 2024

  • Updates Milvus hosting to leverage dynamically generated default password if none provided (similar to other resources) and resulting cascading changes + tests.
  • Updates tags for contianer image that support setting auth

Description

Milvus added the ability to set the root password. This change adapts to that and follows similar pattern to other config-based auth (e.g., MySql is a good analog). This requires the support of a minimum new Milvus version (2.4.7).

Fixes #4966

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected. - haven't seen a pattern where we've made a change that requires a specific new resource version
  • 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 - follows same pattern as existing pattern/resources
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

- Updates Milvus hosting to leverage dynamically generated default password if none provided (similar to other resources) and resulting cascading changes + tests.
- Updates tags for contianer image that support setting auth
@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 9, 2024
@timheuer
Copy link
Member Author

timheuer commented Aug 9, 2024

@eerhardt had to redo the PR (my branch was way stale). Addressed your prior feedback.

Review: this does change resource and enables a null param were there wasn't before. Not sure if we've made a similiar change there -- seeking feedback on desired correct way here.

@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2024

Review: this does change resource and enables a null param were there wasn't before. Not sure if we've made a similiar change there -- seeking feedback on desired correct way here.

I believe you did it correctly. 👍

@radical
Copy link
Member

radical commented Aug 9, 2024

Would this allow adding Milvus playground app to the tests like in #5244 ? When I had tried it earlier on main it was failing with Parameter resource could not be used because configuration key 'Parameters:milvusauth' is missing and the Parameter has no default value..

@eerhardt
Copy link
Member

eerhardt commented Aug 9, 2024

Would this allow adding Milvus playground app to the tests like in #5244 ?

Yes, it should.

When I had tried it earlier on main it was failing with Parameter resource could not be used because configuration key 'Parameters:milvusauth' is missing and the Parameter has no default value..

I thought we generated random parameter values:

public static TBuilder WithRandomParameterValues<TBuilder>(this TBuilder builder)
where TBuilder : IDistributedApplicationTestingBuilder
{
var parameters = builder.Resources.OfType<ParameterResource>().Where(p => !p.IsConnectionString).ToList();
foreach (var parameter in parameters)
{
builder.Configuration[$"Parameters:{parameter.Name}"] = parameter.Secret
? PasswordGenerator.Generate(16, true, true, true, false, 1, 1, 1, 0)
: Convert.ToHexString(RandomNumberGenerator.GetBytes(4));
}

Isn't that code supposed to fix this error?

- Removed tests from testproject (handled in func)
- changed tests to enable the random parameter usage
@timheuer
Copy link
Member Author

Any other feedback @eerhardt?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice work! Much easier to get started with Milvus.

@eerhardt eerhardt merged commit 89fbdc0 into main Aug 13, 2024
@eerhardt eerhardt deleted the timheuer/4966 branch August 13, 2024 22:05
@github-actions github-actions bot locked and limited conversation to collaborators Sep 15, 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.

Modify Milvus hosting resource to use generated password by default

4 participants