Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add comment for cosmosdb.go tests #8477

Merged
merged 7 commits into from
Jan 9, 2025
Merged

Add comment for cosmosdb.go tests #8477

merged 7 commits into from
Jan 9, 2025

Conversation

itaigilo
Copy link
Contributor

@itaigilo itaigilo commented Jan 8, 2025

Re #8476.

@itaigilo itaigilo added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Jan 8, 2025
@itaigilo itaigilo requested review from N-o-Z and idanovo January 8, 2025 15:41
idanovo
idanovo reviewed Jan 8, 2025
View reviewed changes
pkg/testutil/cosmosdb.go Outdated
@@ -61,6 +61,7 @@ func GetCosmosDBInstance() (string, func(), error) {

dockerPool.MaxWait = maxWait
log.Printf("Waiting up to %v for emulator to start", dockerPool.MaxWait)
// Note: this hangs for Apple chip users, and fails. See https://github.com/treeverse/lakeFS/issues/8476
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I recommend going with @N-o-Z suggestion to skip this test for Mac users by adding this if statement:
if runtime.GOOS == "darwin" {
    // skip test with message
}
  1. I don't think this is an Apple chip issue, this is an OS issue

idanovo
idanovo approved these changes Jan 8, 2025
View reviewed changes
Copy link
Contributor

@idanovo idanovo left a comment

Choose a reason for hiding this comment

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

I don't want to block this, but please take a look at my comment before merging 🙏

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025

E2E Test Results - Quickstart

11 passed

@N-o-Z
Copy link
Member

N-o-Z commented Jan 9, 2025

@itaigilo Why did we revert the code that skips the test for Mac?

@N-o-Z
Copy link
Member

N-o-Z commented Jan 9, 2025

@itaigilo You need to rebase over latest master

@itaigilo itaigilo merged commit b46e8da into master Jan 9, 2025
38 checks passed
@itaigilo itaigilo deleted the chore/comment-about-cosmodb-failing-test branch January 9, 2025 18:42
itaigilo added a commit that referenced this pull request Jan 9, 2025
@itaigilo
Revert "Add comment for cosmosdb.go tests (#8477)"
f538f30 
This reverts commit b46e8da.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@idanovo idanovo idanovo approved these changes

@N-o-Z N-o-Z Awaiting requested review from N-o-Z

Assignees
No one assigned
Labels
exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@itaigilo @N-o-Z @idanovo