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

[130] Fixed comparison to make sure multipart_global_state is cleaned up #134

Merged
merged 1 commit into from
Nov 11, 2024
Merged

[130] Fixed comparison to make sure multipart_global_state is cleaned up #134

merged 1 commit into from
Nov 11, 2024

Conversation

JustinKyleJames
Copy link
Contributor

This is to fix issue 130.

All tests passed.

korydraughn
korydraughn reviewed Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Code change looks good.

Is it possible to write a test for this?

@JustinKyleJames
Copy link
Contributor Author

Code change looks good.

Is it possible to write a test for this?

It is but I don't believe I have any tests for AbortMultipartUpload at the moment. I can add a test to do exactly what was causing the problem before.

Once ListParts and ListMultipartUploads are implemented I will able to have better tests.

@korydraughn
Copy link
Collaborator

Yes, add a test which shows that the thing which triggered the issue is now resolved.

We'll merge this once that's added.

@JustinKyleJames
Copy link
Contributor Author

I added a test case to test the scenario that was failing. I verified that the new test case passes with the code change and fails without the code change.

alanking
alanking reviewed Oct 9, 2024
View reviewed changes
Copy link

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Just had a question, but I think this is good to go. Can go ahead and squash if desired - leaving separate commits is also fine, IMO.

tests/abortmultipartupload_test.py Show resolved Hide resolved
@JustinKyleJames
Copy link
Contributor Author

Is this ready to be squashed? I know Alan stated it was but there were more review comments/changes after that.

@korydraughn
Copy link
Collaborator

If the work has been completed, please squash it.

@alanking
Copy link

I do not see the new changes yet. Perhaps they have not been pushed?

@JustinKyleJames
Copy link
Contributor Author

If the work has been completed, please squash it.

I squashed

korydraughn
korydraughn approved these changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Let's wait for one more approval before pounding.

alanking
alanking approved these changes Nov 11, 2024
View reviewed changes
Copy link

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Nice. # it

@JustinKyleJames
Copy link
Contributor Author

Nice. # it

done

@alanking alanking merged commit efdad5b into irods:main Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trel trel trel left review comments

@alanking alanking alanking approved these changes

@korydraughn korydraughn korydraughn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JustinKyleJames @korydraughn @alanking @trel