Skip to content

Conversation

@Alirexaa
Copy link
Contributor

@Alirexaa Alirexaa commented Sep 15, 2024

Description

Adds WaitFor support for Elasticsearch

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-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Sep 15, 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 15, 2024
@davidfowl davidfowl added area-integrations Issues pertaining to Aspire Integrations packages and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Sep 16, 2024
@mitchdenny
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mitchdenny
Copy link
Member

Looks good to me (based on our current pattern).

@davidfowl
Copy link
Member

The timeout in the test might be too low. I don't know how long elastic search takes to boot but its notoriously slow and unreliable from our other tests?

@mitchdenny mitchdenny merged commit f177eb5 into dotnet:main Sep 17, 2024
11 checks passed
@davidfowl
Copy link
Member

cc @radical note the changes

@Alirexaa Alirexaa deleted the waitfor-elasticsearch branch September 17, 2024 15:29
<_DefaultWorkItems TimeoutMs="900000" />

<_DefaultWorkItems Condition="'%(FileName)' == 'Aspire.Hosting.Elasticsearch.Tests'" TimeoutMs="1200000" />
<_DefaultWorkItems Condition="'%(FileName)' == 'Aspire.Hosting.Elasticsearch.Tests'" TimeoutMs="3600000" />
Copy link
Member

Choose a reason for hiding this comment

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

Is this really an hour? We are going to wait an hour for these tests? Isn't that just going to make us wait longer when the thing isn't responsive?

Copy link
Member

Choose a reason for hiding this comment

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

This will become a problem whenever helix is backed up. The total timeout for the whole job is 90mins, so this will definitely timeout. Do we need to break this up?

# timeout accounts for wait times for helix agents up to 30mins
timeoutInMinutes: 90

Copy link
Member

Choose a reason for hiding this comment

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

The test run on this PR took ~8mins. If this is taking up to an hour sometimes then maybe we need to investigate why that is the case.

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

5 participants