[GAL-337] Handle 'false-conflicts' in upgrade diff #308
Conversation
|
Looks like it would be good to duplicate the tests for print-only-conflicts enabled and disabled. |
|
I added tests to check the logs. The current userchanges tests aren't affected by the option as this will change the log output, so I thought it's easier to add a new test case that'll capture the possible log changes |
| } | ||
|
|
||
| protected List<String> expectedDiff() { | ||
| return null; |
There was a problem hiding this comment.
Could this be moved to UserChangesTestBase, assuming no other test needs this?
There was a problem hiding this comment.
I moved the changes to UserChangesTestBase now
| @@ -124,7 +124,6 @@ protected DirState provisionedHomeDir() { | |||
| return newDirBuilder() | |||
| .addFile("prod1/p1.txt", "prod1 p1") | |||
| .addFile("prod1/p2.txt", "user") | |||
| .addFile("prod1/p2.txt.glnew", "prod1 p2") | |||
There was a problem hiding this comment.
Could these files be modified in a way that create a conflict just to preserve these tests? They seem to be valuable as they are. It'd be better to add new tests for the feature you are introducing.
There was a problem hiding this comment.
I think this would require creating an updated prod1 FP and installing the updated package on top of prod2 - similarly to UserChangesAfterUpdatePlanTestCase
Alternatively prod2 could override the prod1/p2.txt file, but that use case is already captured in common.txt.
There was a problem hiding this comment.
Ah, if it's already covered then we are good. Thanks.
There was a problem hiding this comment.
@spyrkob could you please confirm the rest of the existing tests are also not undermined by these changes?
There was a problem hiding this comment.
I've added a couple cases to UserChangesAfterUndoTestCase and UserChangesAfterUninstallTestCase
- a common file provisioned by
prod1andprod2and modified by user before undo should generate a diff (replacesprod1/p2.txtscenario) - a file added in
prod2is modified by the user and should be preserved (not tested before)
Changes in UserChangesAfterMultipleUpdatesTestCase are covered by UserChangesAfterUpdatePlanTestCase (file changes between two versions of FP and is modified by user). The difference is the '.glnew' files are not present because those files are not modified by prod2 feature pack. I'm wondering if it's worth replacing prod2 with prod102 which would further change the files of prod1 and prod2
| @@ -134,8 +137,6 @@ public static Map<String, Boolean> replay(FsDiff diff, Path home, MessageWriter | |||
| } | |||
| } else if (modifiedPathConflict(update)) { | |||
| glnew(target); | |||
| } else { | |||
There was a problem hiding this comment.
I am wandering if we should introduce a new kind of action SAFE_MODIFICATION, otherwise this change is still reported as a conflict [C] (when printOnlyConflicts is false) although it is not and we are not generating glnew file. When reporting the conflict we should check if the action is not a SAFE_MODIFICATION: action != SAFE_MODIFICATION && (!printOnlyConflicts || warning != null).
There was a problem hiding this comment.
Maybe I misunderstood the desired outcome.
I thought the goal was to print the warnings as they are now (without printOnlyConflicts) and skip the .glnew file on false conflicts. Or do we want to remove those from logs entirely? If we remove those from logs - should we leave non-conflicting addition/removal?
Or do you mean we should report those as modified [M] rather then conflict [C], signifying that the user's changes have been preserved but there's no need for user input (sort of like [+]/[-])?
There was a problem hiding this comment.
Advertising a conflict that is not a conflict seems strange to me, reporting with a [M] is a good idea. That would answer my concern.
There was a problem hiding this comment.
Added Modified [M] state to the diff outcome. Also changed get-changes command to use [M] instead of [C] because it detects files with user changes not conflicts with next version.
False-conflicts don't generate .glnew files Added 'print-only-conflicts' option to allow displaying only real conflicts
Proposal for https://issues.redhat.com/browse/GAL-337
False-conflicts don't generate .glnew files
Added 'print-only-conflicts' option to allow displaying only real conflicts