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

Verify Indices Mappings and Clear all Indices #1064

Conversation

bodo-hugo-barwich
Copy link
Contributor

@bodo-hugo-barwich bodo-hugo-barwich commented Jul 30, 2022

This implements the functionality discussed at:
Fix Corrupted Indices and Verify Mappings
It is also able to fix difficulties and corruptions as documented at
Unexpected Index Corruption

Additionally it implements Exit with Error Code for unconfirmed Operations.

To test all the functionality it features several Mapping Script Tests.

lib/MetaCPAN/Script/Mapping.pm Outdated Show resolved Hide resolved
@mohawk2
Copy link
Contributor

mohawk2 commented Aug 13, 2022

This is a big (adding 700 lines) change that introduces valuable functionality. My only observation is it seems splittable conceptually into two pieces of functionality; the verify, and the clear-indices. Would it make sense to make a separate PR that only introduced the checking/verification code from this? That would be a smaller, much less risky change; then this one could be rebased past that, with the higher-risk code isolated and more easily reviewable.

@bodo-hugo-barwich
Copy link
Contributor Author

Yes, you are right. I also noticed that I actually was working on two different tasks. But unfortunately both share the same code and also the same tests. So I need to rebuild a separate branch from the master branch and reimport the sub routines from the current branch, and review it of course. Also the second branch would need to be rebased according to which branch is merged first.

@bodo-hugo-barwich
Copy link
Contributor Author

I built a new branch at:
Clear Unknown Indices
which implements the --delete --all option and also introduces important tests which are used for the mappings verification tests.

@mohawk2
Copy link
Contributor

mohawk2 commented Aug 20, 2022

I would really like to see the verify done (including merged and released, since it is inherently less risky) first, not the clear. Is there a good reason why it can't be done in that order?

@bodo-hugo-barwich
Copy link
Contributor Author

Actually the verify mappings logic is the bigger part of this development with the introducing the _compare_mapping(), _build_mapping() and _build_aliases() methods with more than 200 lines of code additionally to the tests with another 100 lines of code.
The tests of the verify mappings logic require tests of --create and --update index functionality which require some additional 100 lines of changes.
So my idea was to split the code more evenly and already introduce tests in the smaller task that will thin out the verify mappings development which supposedly would increase the maintainability and reviewabilty of the development.

On the other side the tests of the --delete --all logic show that this functionality is save and should not be able to run in production if my assumptions about the environment recognition are as documented in the related issue:
recognize development environment
flag development environment with environment variables
Still there is also a manual protection as requested at:
manual confirmation required

@bodo-hugo-barwich
Copy link
Contributor Author

I rebuilt the "Verify Mappings" functionality at:
Verify Indices Mappings
which is the semantical split of this development.

@mickeyn
Copy link
Contributor

mickeyn commented Dec 23, 2022

the other PR is merged, so shall we close this one?

@bodo-hugo-barwich
Copy link
Contributor Author

this development was split into 2 separate Pull Requests

so this Pull Request is obsolete and can be closed

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.

4 participants