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

fix(volumes): some fixes and cleanups #359

Merged
merged 7 commits into from
May 23, 2024
Merged

fix(volumes): some fixes and cleanups #359

merged 7 commits into from
May 23, 2024

Conversation

tty47
Copy link
Contributor

@tty47 tty47 commented May 22, 2024

hello!

Fixes: #358

@tty47
fix(volumes): some fixes and cleanups
cdfb1bb 
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 self-assigned this May 22, 2024
@tty47 tty47 requested a review from a team May 22, 2024 10:48
@celestia-bot celestia-bot requested review from MSevey, smuu and sysrex May 22, 2024 10:49
@tty47 tty47 added the fix Fix label May 22, 2024
mojtaba-esk
mojtaba-esk previously approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

Thanks, well done

e2e/basic/file_test_image_cached_test.go Outdated Show resolved Hide resolved
e2e/basic/folder_test_image_cached_test.go Outdated Show resolved Hide resolved
@tty47
fix(volumes): cleanup
a0f940d 
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 requested a review from mojtaba-esk May 22, 2024 11:12
smuu
smuu previously approved these changes May 22, 2024
View reviewed changes
@tty47 tty47 enabled auto-merge May 22, 2024 13:46
MSevey
MSevey requested changes May 22, 2024
View reviewed changes
Copy link
Member

@MSevey MSevey left a comment

Choose a reason for hiding this comment

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

Please refactor into a helper since this was copy pasted into 5 plus tests.
It is reasonable to believe that future tests will need this same functionality so we should have a helper to DRY up the code.

mojtaba-esk
mojtaba-esk previously approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

💯

@tty47
fix(comments): add assert func
e6deacf 
Signed-off-by: Jose Ramon Mañes <[email protected]>
@tty47 tty47 dismissed stale reviews from mojtaba-esk and smuu via e6deacf May 23, 2024 11:08
@tty47 tty47 requested review from MSevey, mojtaba-esk and smuu May 23, 2024 11:44
@tty47
Copy link
Contributor Author

tty47 commented May 23, 2024

Please refactor into a helper since this was copy pasted into 5 plus tests. It is reasonable to believe that future tests will need this same functionality so we should have a helper to DRY up the code.

thanks for the review, I've added an assert function to create/commit the instances and don't repeat this block of code

mojtaba-esk
mojtaba-esk approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@mojtaba-esk mojtaba-esk left a comment

Choose a reason for hiding this comment

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

Well done. now the code is cleaner

@MSevey MSevey disabled auto-merge May 23, 2024 18:00
@MSevey MSevey merged commit 2981a2a into main May 23, 2024
3 of 4 checks passed
@MSevey MSevey deleted the jose/fix-tests-volumes branch May 23, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smuu smuu smuu approved these changes

@MSevey MSevey MSevey approved these changes

@mojtaba-esk mojtaba-esk mojtaba-esk approved these changes

@sysrex sysrex Awaiting requested review from sysrex

Assignees

@tty47 tty47

Labels
fix Fix
Projects
Celestia DevOps/Testing
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue with the volumes
4 participants
@tty47 @mojtaba-esk @MSevey @smuu