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

fixed issue of Backup tool not correctly detecting .maintenance.flag #19993

Merged
merged 4 commits into from
Apr 23, 2019

Conversation

hiren0241
Copy link
Contributor

@hiren0241 hiren0241 commented Dec 27, 2018

Fixed issue of Backup tool not correctly detecting .maintenance.flag

Description (*)

This PR fix the issue of backup tool not correct detecting .maintenance.flag issue in Magento 2.3.0.

Fixed Issues (if relevant)

  1. Magento 2.3.0: Backup tool not correctly detecting .maintenance.flag #19825: Magento 2.3.0: Backup tool not correctly detecting .maintenance.flag

Manual testing scenarios (*)

Case 1:

  1. Go to Dashboard -> System -> Tools -> Backups -> Database Backup
  2. Enter backup name, check "Maintenance mode", click OK.
  3. Back up is created

Case 2:

  1. Go to Dashboard -> System -> Tools -> Backups -> Database Backup
  2. Enter backup name, and click OK.
  3. Back up is created.

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @hiren0241. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

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

@hiren0241
Copy link
Contributor Author

Hi @sidolov ,
Will you please guide me how can I pass travis check? As I am unable to debug where it fails.

@orlangur
Copy link
Contributor

@hiren0241 what do you mean? You can see failed unit tests here: https://travis-ci.org/magento/magento2/jobs/472906682

@hiren0241
Copy link
Contributor Author

Hi @orlangur,

Thanks for your response.

I did analysis on error but I didn't change anything in unit testing or for that particular stuff but still it's showing CI fails. SO, I am confused which thing preventing the build.

One more thing, In travis job log it's suggest below listed things:

  1. Magento\Backup\Test\Unit\Controller\Adminhtml\Index\CreateTest::testExecutePermission
    Expectation failed for method name is equal to "get" when invoked zero or more times
    Parameter 0 for invocation Magento\Framework\ObjectManagerInterface::get('Psr\Log\LoggerInterface') does not match expected value.
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'Magento\Backup\Helper\Data'
    +'Psr\Log\LoggerInterface'
    /home/travis/build/magento/magento2/app/code/Magento/Backup/Controller/Adminhtml/Index/Create.php:94
    /home/travis/build/magento/magento2/app/code/Magento/Backup/Test/Unit/Controller/Adminhtml/Index/CreateTest.php:238

But, this is default magento stuff. What's wrong with it that I can't understand. So, I am asking for help.

Please share your valuable knowledge on this so that I can resolve this issue.

Thanks in advance.

@orlangur
Copy link
Contributor

@hiren0241 you changed code covered by this test, it means that either changes (if they do not work for all cases) or the test should be adjusted so that they match each other again.

@hiren0241
Copy link
Contributor Author

Thanks @orlangur,

I understand. Will you please review my changes so that I can get assurance that my changes fix the issue but now I have to look into unit testing stuff. Is it possible for you to review it or I have to fix travis stuff first?

Please do the needful.

@magento-engcom-team
Copy link
Contributor

@hiren0241 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@hiren0241
Copy link
Contributor Author

@magento-engcom-team Is My PR accepted? or should I have to update test code by myself?

@sivaschenko
Copy link
Member

@hiren0241 you need to adjust the failed unit test to your changes. Please let me know if you need any assistance on fixing unit tests. Feel free to reach me i.e. in magentocommeng slack.

@sivaschenko
Copy link
Member

Discussed the test fail with @hiren0241 and decided to try to rework the fix shifting attention towards \Magento\Framework\App\MaintenanceMode::set method return.

@sidolov
Copy link
Contributor

sidolov commented Feb 6, 2019

Hi @hiren0241 , please, take a look at failed tests

@travis5491811
Copy link

I noticed you all are working on backup tool. Is it not really deprecated? If not, any idea why docs are saying it is?

Deprecation Notice! Magento backup features are deprecated as of v2.3.0. We recommend that all merchants investigate additional backup technologies and binary backup tools (such as Percona XtraBackup).

@sivaschenko
Copy link
Member

Closing this pull request due to inactivity, feel free to reopen if you'd like to continue progress

@ghost
Copy link

ghost commented Feb 21, 2019

Hi @hiren0241, 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.

@hiren0241
Copy link
Contributor Author

@sivaschenko Hi,
I am reopening this PR, As I would like to continue working upon it.

}
$this->maintenanceMode->set(true);
} else {
$response->setError(
Copy link
Member

Choose a reason for hiding this comment

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

Displaying insufficient permissions error in case maintenance_mode parameter is false is logically incorrect

@hiren0241
Copy link
Contributor Author

Hi @stoleksiy ,
Author of issue has didn't mention regarding rollback, And even though this fixed is only for create back up not for rollback.

If you want this changes for rollback as well, than I will apply same logic over there.

Please guide me for the same.

Thanks in advance.

@soleksii
Copy link

@hiren0241 Thanks for the answer! Could you please make these changes in scope of this PR?

@hiren0241
Copy link
Contributor Author

@stoleksiy , Sure, I Will do the same. Will update you in sometime after commit the changes.

@soleksii
Copy link

@hiren0241 Thanks!

@hiren0241
Copy link
Contributor Author

@stoleksiy,

Please re-test it now.

Thanks.

@hiren0241
Copy link
Contributor Author

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @hiren0241. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, thank you for the review.
ENGCOM-4782 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @hiren0241, here is your new Magento instance.
Admin access: https://pr-19993.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@soleksii
Copy link

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Apr 23, 2019

Hi @hiren0241, 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.2 milestone Apr 23, 2019
taskula pushed a commit to Hypernova-Oy/magento2 that referenced this pull request Apr 23, 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.

9 participants