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.

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

Delete operation should only delete attributes which exist in the dictionary #661

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vkuznet
Copy link
Contributor

@vkuznet vkuznet commented Jul 28, 2022

This PR provides an additional check before deleting certain attributes in file parent dictionaries which may be omitted in the output of blockdumps API. In particular, the newly deployed Go server does not provide parent_file_id in its output, since this information is redundant (i.e. it cannot be used in migration process since these ids have no sense in destination database, and moreover python DBSMigration code does not use this information either). As such, I address this issue in this PR to make it compatible with new Go-based DBS server. We may need it during transition period when we would like to run
side by side both DBSMigration servers.

@vkuznet
Copy link
Contributor Author

vkuznet commented Jul 28, 2022

@amaltaro , I would like you to have a look at this PR as I want to make Python migration server compatible with new Go-based server where parent_file_id information is not provided (since it is redundant. The actual issue about this is here dmwm/dbs2go#70 and we need to decide which approach to take, see my reply here dmwm/dbs2go#70 (comment) I'll comeback to this issue one I back from vacation.

Copy link
Contributor

@amaltaro amaltaro left a comment

Choose a reason for hiding this comment

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

Valentin, I didn't read all the discussion that you pointed out.
However, to me your change doesn't affect any behavior and it will actually have a safer key deletion. I left a suggestion in the code though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants