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

Bug 30744239 RemoveInstalledCriteria does not remove all matched entries #70

Closed
wants to merge 1 commit into from

Conversation

jw-msft
Copy link
Contributor

@jw-msft jw-msft commented May 26, 2021

Allow RemoveInstalledCriteria to remove duplicates.

Add tests for removing all dups and not removing non-matching.
Add InstalledCriteriaPersistence RAII class and used it to remove ADUC_INSTALLEDCRITERIA_FILE_PATH on destruction to improve test isolation for tests that use PersistInstalledCriteria().

Update docs for RemoveInstalledCriteria with example json.

screenshot of doxygen:
image

@jw-msft jw-msft requested review from JeffMill, Nox-MSFT and a team May 26, 2021 09:19
Copy link
Contributor

@Nox-MSFT Nox-MSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@jw-msft jw-msft requested a review from Nox-MSFT May 26, 2021 17:30
{
break;
}
// otherwise, keep going to remove duplicates
}
Copy link
Contributor

@JeffMill JeffMill May 26, 2021

Choose a reason for hiding this comment

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

do we really want to serialize after every remove (line 500)? seems like it would be better to move 500-501 outside of the loop. the fact that the loop on 489 is going in reverse order seems to imply that is the intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! I will be sure to address this. I am closing this PR and will replicate to align with repo/branching strategy.

Copy link
Contributor

@JeffMill JeffMill left a comment

Choose a reason for hiding this comment

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

🕐

@jw-msft jw-msft closed this May 26, 2021
@jw-msft jw-msft deleted the user/jw-msft/rmvDupInstalledCrit branch February 10, 2022 06:28
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