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

Fix unexpected diff between builds in generated/metadata directory #23325

Conversation

ihor-sviziev
Copy link
Contributor

Description (*)

Sort all path definitions before generating config and writing it to generated/metadata directory

This issue caused by using RecursiveDirectoryIterator object for generating list of all classes and all their arguments and it could return not sorted result. As result files between builds are different just because files were found in different sort order.

Fixed Issues (if relevant)

  1. [Reproducible Builds] Diff in generated/metadata between the builds #23324: [Reproducible Builds] Diff in generated/metadata between the builds

Manual testing scenarios (*)

  1. See [Reproducible Builds] Diff in generated/metadata between the builds #23324 for details

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Jun 19, 2019

Hi @ihor-sviziev. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

Sort all path definitions before generating config
@ihor-sviziev ihor-sviziev force-pushed the 2.3-fix-sorting-in-generated-metadata branch from 71f586f to d8486ba Compare June 19, 2019 15:18
@orlangur orlangur self-assigned this Jun 19, 2019
@sivaschenko
Copy link
Member

@orlangur Any updates on this pull request? Why is it on hold?

@orlangur
Copy link
Contributor

@sivaschenko because of discussion in corresponding issue, need to check how long

$this->sortDefinitions($definitionsCollection);
will take, will review this week.

@ihor-sviziev
Copy link
Contributor Author

Hi @orlangur,

This PR was created almost 2 months ago. There is no clear description why this PR should be kept in "on hold" such big amount of time. If you think there might be performance degradation issues - QAs could re-check it. I checked it myself and there is no big difference.

I do understand that you could not have enough time for reviewing this PR, but I think it's not a good think to holding PR when you don't have time for reviewing it.

@sivaschenko @sidolov could you review my PR?

Thank you!

@orlangur
Copy link
Contributor

@ihor-sviziev the only concern was performance.

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5592 has been created to process this Pull Request
✳️ @orlangur, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@m2-assistant
Copy link

m2-assistant bot commented Aug 27, 2019

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants