removing conflits (#387)

[michalwedzik]

Description

Related Issue

#387

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have signed the Adobe Open Source CLA.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[rombert]

This is a tricky one :-)

First comment is that we don't know about SKYOPS here, so you need to an issue created in this project, describing the problem and the fix.

The second comment is that the conflict removal is brittle. The logic is "if there are any conflicts, remove conflicts that we know are problematic". As the analyser in Sling evolves we may run into false positives here and remove unintended statements.

I think it would be best to get the conflicting statements from the RepoInitConflictsValidator , e.g. List<List<RepoinitStatement>>, where the inner list contains the (probably) two conflicting statements, e.g.

• create path (sling:Folder) /apps/foo/clientlibs/js
• create path (sling:Folder) /apps/foo/clientlibs(cq:ClientLibraryFolder)/js

so you know for sure you'll remove the right statement and be able to skip those that we can't handle.

Final comment is a question - what is the logging behaviour when this runs? Ideally we would have something like:

• if a conflict is found in the validator but not handled by RepoInitUtil - original WARN from the validator
• if a conflict is found in the validator but not handled by RepoInitUtil - new INFO that we removed the duplicate but no WARN from the validator

[michalwedzik]

First comment is that we don't know about SKYOPS here, so you need to an issue created in this project, describing the problem and the fix.

I'll create

I think it would be best to get the conflicting statements from the RepoInitConflictsValidator , e.g. List<List>, where the inner list contains the (probably) two conflicting statements, e.g.

Good suggestion, thanks. I’ll improve it and see how it looks.

Final comment is a question - what is the logging behaviour when this runs? Ideally we would have something like:

if a conflict is found in the validator but not handled by RepoInitUtil - original WARN from the validator
if a conflict is found in the validator but not handled by RepoInitUtil - new INFO that we removed the duplicate but no WARN from the validator

The mechanism is fully independent. The validator doesn’t know about RepoinitUtil. If we want to implement it as above, the validator would need to know which paths should be removed, and also whether conflict removal is enabled.

[rombert]

Final comment is a question - what is the logging behaviour when this runs? Ideally we would have something like:
if a conflict is found in the validator but not handled by RepoInitUtil - original WARN from the validator
if a conflict is found in the validator but not handled by RepoInitUtil - new INFO that we removed the duplicate but no WARN from the validator

The mechanism is fully independent. The validator doesn’t know about RepoinitUtil. If we want to implement it as above, the validator would need to know which paths should be removed, and also whether conflict removal is enabled.

Yes, that is correct. If we enable the validator using the DEFAULT_TASKS approach then the validator will run and then we also want to fix it using RepoInitUtil → double logging.

We could, for example, remove the validator from DEFAULT_TASKS if conflict removal is enabled by making that list dynamic. Or we could not register that validator and register a different one implemented here that extends it and, as part of the validation, handles the conflict removal.

[michalwedzik]

Final comment is a question - what is the logging behaviour when this runs? Ideally we would have something like:
if a conflict is found in the validator but not handled by RepoInitUtil - original WARN from the validator
if a conflict is found in the validator but not handled by RepoInitUtil - new INFO that we removed the duplicate but no WARN from the validator

The mechanism is fully independent. The validator doesn’t know about RepoinitUtil. If we want to implement it as above, the validator would need to know which paths should be removed, and also whether conflict removal is enabled.

Yes, that is correct. If we enable the validator using the DEFAULT_TASKS approach then the validator will run and then we also want to fix it using RepoInitUtil → double logging.

We could, for example, remove the validator from DEFAULT_TASKS if conflict removal is enabled by making that list dynamic. Or we could not register that validator and register a different one implemented here that extends it and, as part of the validation, handles the conflict removal.

I don't think validation should modify the data. I'll try to implement a dynamic list of validators, or at least explore what that approach would look like.

[michalwedzik]

@rombert,

Yes, that is correct. If we enable the validator using the DEFAULT_TASKS approach then the validator will run and then we also want to fix it using RepoInitUtil → double logging.

If I understand the process correctly, analysis and aggregation are only triggered from the ConvertMergeAnalyseCommand in the fact tool.

First, aggregation is performed, and that's where I remove the conflicts (nothing is logged at this stage). After aggregation, the analysis runs and tries to find conflicts.

So, if conflict removal is enabled, the analyser won't log any conflicts because they'll already have been removed.

In that case, I should only log that conflicts were removed, so there won't be any duplicate logging.

Am I understanding this correctly?

[rombert]

If I understand the process correctly, analysis and aggregation are only triggered from the ConvertMergeAnalyseCommand in the fact tool.

Right. More generally, it will run whenever the AemAggregator is invoked with the specific flag set.

First, aggregation is performed, and that's where I remove the conflicts (nothing is logged at this stage). After aggregation, the analysis runs and tries to find conflicts.

So, if conflict removal is enabled, the analyser won't log any conflicts because they'll already have been removed.

In that case, I should only log that conflicts were removed, so there won't be any duplicate logging.

That sounds correct, yes.

Just for the sake of being complete: if the analyser finds conflicts that are not handled by your specific changes then they will be always logged, because we don't try to solve all conflicts here.

Additionally, please remove the SKYOPS reference from this PR and create an issue in this GitHub repo.

[michalwedzik]

@rombert,

• I added additional logging.
• I removed SKYOPS and created a ticket in this repository: Incorrect repoinit statements generated for bundles embedded more than once at different paths #387
• Before merging it, I need to update sling-org-apache-sling-feature-analyser first, so I moved this ticket to draft.
• After updating the version of sling-org-apache-sling-feature-analyser, I'll remove the conflicts.