-
Notifications
You must be signed in to change notification settings - Fork 120
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
Unify calls to list directory contents #695
Unify calls to list directory contents #695
Conversation
6076961
to
18267e0
Compare
Guys please make sure we don't reintroduce some issues previously fixed, like - #651 🙏 |
The test I'm adding is supposed to cover the case you are talking about in your link, even if we're talking about autoupgrade/tests/fixtures/listOfFiles-ReleaseExample-upgrade-with-theme.json Lines 21 to 25 in 05d7e94
There is another case that could be covered though, where the case can be different (i.e |
05d7e94
to
367f0b1
Compare
@Quetzacoalt91 Cool, good job!!! :-) |
2935c36
to
f5bd50f
Compare
One idea @Quetzacoalt91, do you think it would be to use this opportunity to fix one annoying behavior? The current logic copies all of the native modules in the zip to the install. Maybe we could copy them only if the folder already exists? cc @MatShir and @kpodemski |
04e1cc4
to
b980555
Compare
There are a few improvement we'd like to add in the upgrade process, something a bit smarter when we deal about modules (i.e avoiding rollback while copying, check if a module is new in the new release vs a simple upgrade). This PR makes us ready to stop copying the modules folder once we have something ready to improve module management during an upgrade. |
b980555
to
df2af67
Compare
df2af67
to
b9f772d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR, I tested it and it seems to works as you can see :
recording.214.webm
Tested on :
8.0.5 to 8.1.6 from major release, I've done a rollback after this
8.1.5 to 8.1.6 from minor release, I've done a rollback after this
8.0.5 to 9.0.0 from local release
8.1.5 to 9.0.0 from local release
1.7.8.11 to 8.1.6 from major release
Because the PR seems to works as expected, It's QA ✔️
Thank you
To Do list
Ignore modules during upgradeRevertedKnown issues
This will be fixed when the upgrade of modules is improved. In the meantime ignoring the whole modules/ folder has been reverted.
Issue found with the current version