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

Added replace_objects and replace_container_contents options #16

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Added replace_objects and replace_container_contents options #16

merged 3 commits into from
Mar 26, 2024

Conversation

MaxGelbakhiani
Copy link
Contributor

@MaxGelbakhiani MaxGelbakhiani commented Mar 12, 2024 •
edited
Loading

Fixes #2 and #15

@MaxGelbakhiani MaxGelbakhiani requested review from evgeniiz321 and removed request for vvarg229 March 12, 2024 12:48
@roman-khimov roman-khimov mentioned this pull request Mar 12, 2024
Closed
roman-khimov
roman-khimov requested changes Mar 12, 2024
View reviewed changes
action.yml Outdated Show resolved Hide resolved
push-to-neofs.py Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
push-to-neofs.py Show resolved Hide resolved
push-to-neofs.py Show resolved Hide resolved
push-to-neofs.py
attributes,
base_cmd,
put_timeout,
files = get_file_info(directory, url_path_prefix)
Copy link
Member

Choose a reason for hiding this comment

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

Suboptimal to me, the set can be arbitrarily big and caching it all at once is not required both for #2 and #15.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the same approach introduced in the original code, but the os.walk() part is now in a function since I need the file list more than once.

push-to-neofs.py Show resolved Hide resolved
@MaxGelbakhiani MaxGelbakhiani marked this pull request as draft March 12, 2024 19:47
@MaxGelbakhiani MaxGelbakhiani changed the title Added replace_existing option Added replace_objects option Mar 12, 2024
@MaxGelbakhiani MaxGelbakhiani force-pushed the added_replace_existing_flag branch 2 times, most recently from d8bbd63 to 299e8b5 Compare March 13, 2024 06:46
@MaxGelbakhiani MaxGelbakhiani changed the title Added replace_objects option Added replace_objects and remove_container_contents options Mar 13, 2024
@MaxGelbakhiani MaxGelbakhiani force-pushed the added_replace_existing_flag branch 4 times, most recently from 6669724 to 0908e4e Compare March 13, 2024 08:38
@MaxGelbakhiani MaxGelbakhiani marked this pull request as ready for review March 13, 2024 08:40
roman-khimov
roman-khimov reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks OK. How about testing these parameters in our tests?

action.yml Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

Please also mention #15, btw.

@MaxGelbakhiani
Copy link
Contributor Author

@roman-khimov How do we want the action to behave by default with these flags? Right now the default behavior is to upload (old behavior) without deleting objects.

@roman-khimov
Copy link
Member

The least surprising is likely REPLACE_OBJECTS=True. Can be the default.

@MaxGelbakhiani MaxGelbakhiani changed the title Added replace_objects and remove_container_contents options Added replace_objects and replace_container_contents options Mar 13, 2024
@MaxGelbakhiani MaxGelbakhiani force-pushed the added_replace_existing_flag branch 3 times, most recently from e73ee4a to e64fd05 Compare March 19, 2024 09:32
@MaxGelbakhiani MaxGelbakhiani force-pushed the added_replace_existing_flag branch 15 times, most recently from d5d1440 to c4526d8 Compare March 19, 2024 21:01
roman-khimov
roman-khimov reviewed Mar 23, 2024
View reviewed changes
Copy link
Member

@roman-khimov roman-khimov 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 see a test that does REPLACE_CONTAINER_CONTENTS.

@MaxGelbakhiani
Copy link
Contributor Author

I don't see a test that does REPLACE_CONTAINER_CONTENTS.

Do we have a separate container tied to the current Github repo? Is it safe to rewrite the entire container?

@roman-khimov
Copy link
Member

It's for tests only, it can be rewritten in any way.

@MaxGelbakhiani
Tests for replace_objects option
323706b 
Signed-off-by: MaxGelbakhiani <[email protected]>
@MaxGelbakhiani
Copy link
Contributor Author

It's for tests only, it can be rewritten in any way.

I've pushed tests with container contents replacement. The last 25 minutes test run is the one with deleting all the old objects in the container.

roman-khimov
roman-khimov reviewed Mar 25, 2024
View reviewed changes
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@MaxGelbakhiani
Added test_objects_number
688f6e2 
Signed-off-by: MaxGelbakhiani <[email protected]>
@roman-khimov roman-khimov merged commit d5fcca4 into master Mar 26, 2024
2 checks passed
@roman-khimov roman-khimov mentioned this pull request Mar 26, 2024
Closed
@roman-khimov roman-khimov deleted the added_replace_existing_flag branch March 26, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@evgeniiz321 evgeniiz321 Awaiting requested review from evgeniiz321

Assignees

@roman-khimov roman-khimov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to completely replace container contents
2 participants
@MaxGelbakhiani @roman-khimov