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

Use a deep copy when storing raw node data #124

Merged
merged 2 commits into from
Apr 8, 2021
Merged

Use a deep copy when storing raw node data #124

merged 2 commits into from
Apr 8, 2021

Conversation

psss
Copy link
Collaborator

@psss psss commented Mar 30, 2021

When the adjust() method is used it can modify data referenced
from the raw data dictionaries. Make a deep copy to ensure the
original data are preserved.

  • add test coverage

@psss psss requested a review from lukaszachy March 30, 2021 19:59
@psss psss self-assigned this Mar 30, 2021
Copy link
Collaborator

@jscotka jscotka left a comment

Choose a reason for hiding this comment

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

Hi, seems well.
I've also met this issue, but though that issue is on my side with regerated data from python decorators.
yep, seems like adjust() causes it. BTW, would not be better to fix adjust method if it somehow modify something what it should not modify? e.g. there can be found another issue with method use parameter and reference and potential future problem. and also reason why I've added lines for adjust to test https://github.com/psss/fmf/blob/master/tests/unit/test_modify.py#L82 :-) to prove that it does not modify adjust rule.

@psss
Copy link
Collaborator Author

psss commented Mar 31, 2021

It could be fixed on the adjust() side as well. However, when applying adjustments the adjust attribute is (intentionally) removed. So it is kind-of expected that adjust() modifies the data. Now the question is: Was it a good idea to remove the adjust content after the adjustments are applied? The motivation was: Data can be adjusted just once so let's remove the rules so that possible repeated application does not cause any unexpected/unpredictable changes. Shall we rather keep the rules there? This would also allow tmt test show to more easily show the adjust rules. Thoughts?

@psss
Copy link
Collaborator Author

psss commented Mar 31, 2021

/packit test

@lukaszachy
Copy link
Collaborator

lukaszachy commented Mar 31, 2021

I believe it would be better to allow repetitive adjust calls - tmt object stays "adjusted" until next adjust call.
AFAIK Alex and I need to remember tmt state before adjust (copy method) as our tooling need to call adjust multiple times.
Edit: So having support for multiple adjust right in the fmf/tmt would be very nice.

@jscotka
Copy link
Collaborator

jscotka commented Mar 31, 2021

my feeling is that it should be keep. I can imagine that in pipeline some additional step may change adjust output, e.g. some artificial example :-) I can have centOS and will use RH tool for converting centos to RHEL, and then adjust have to match that it is not centos but rhel (e.g. in prepare phase). I understand that it is little bit artificial, but way, that multiple call of adjustment is not bad. but feature. from py PoV.

And as Lukas mentioned. We need the up to date data, not changed, TMT adjust objects stores the data as well so that removing inside FMF is unnecessary and maybe much more dangerous.

@psss
Copy link
Collaborator Author

psss commented Mar 31, 2021

Makes sense. Let's allow repeated adjust() calls then. Will update the code.

@jscotka
Copy link
Collaborator

jscotka commented Mar 31, 2021

@psss @lukaszachy maybe as quick fix we can also use this patch and create new one (or issue) for your mentioned solution in adjust. Because it will be then also feature not just bugfix.

Copy link
Collaborator

@lukaszachy lukaszachy left a comment

Choose a reason for hiding this comment

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

I've added test, @psss please review.

@psss
Copy link
Collaborator Author

psss commented Apr 8, 2021

Thanks, @lukaszachy. The test looks good. Added changes needed to keep the original adjust rules intact, including a simple test.

@psss psss added this to the 0.16 milestone Apr 8, 2021
@psss
Copy link
Collaborator Author

psss commented Apr 8, 2021

/packit test

@lukaszachy
Copy link
Collaborator

Hm, so integration fails on f34 as tmt changed 'enabled\s+yes' to true

@psss
Copy link
Collaborator Author

psss commented Apr 8, 2021

Yeap, another motivation to find an elegant solution for teemtee/tmt/issues/585 as soon as possible.

psss and others added 2 commits April 8, 2021 13:23
When the `adjust()` method is used it can modify data referenced
from the raw data dictionaries. Make a deep copy to ensure the
original data are preserved. Add test for modify after adjust.

Co-authored-by: Lukáš Zachar <lzachar@redhat.com>
@psss psss merged commit 4e3b969 into master Apr 8, 2021
@psss psss deleted the raw-data-copy branch April 8, 2021 11:59
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.

3 participants