From: AKamyshnikova Date: Thu, 8 Oct 2015 15:18:21 +0000 (+0300) Subject: Per-branch HEAD files for conflict management X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=f144a283ea4f79714b8df288e9eb652e1af7f48d;p=openstack-build%2Fneutron-build.git Per-branch HEAD files for conflict management We have pep8 check for validation revisions, but it allows outdated changes go into merge queue. To prevent this added CONTRACT_HEAD, EXPAND_HEAD files. Closes-bug: #1505701 Change-Id: Ie4b727e55a0b1ecb12e915a0037094a928d8f975 --- diff --git a/doc/source/devref/alembic_migrations.rst b/doc/source/devref/alembic_migrations.rst index 1fdc9aeb9..8762b689e 100644 --- a/doc/source/devref/alembic_migrations.rst +++ b/doc/source/devref/alembic_migrations.rst @@ -312,6 +312,19 @@ following directive should be added in the contraction script:: depends_on = ('',) +HEAD files for conflict management +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In directory ``neutron/db/migration/alembic_migrations/versions`` there are two +files, ``CONTRACT_HEAD`` and ``EXPAND_HEAD``. These files contain the ID of the +head revision in each branch. The purpose of these files is to validate the +revision timelines and prevent non-linear changes from entering the merge queue. + +When you create a new migration script by neutron-db-manage these files will be +updated automatically. But if another migration script is merged while your +change is under review, you will need to resolve the conflict manually by +changing the ``down_revision`` in your migration script. + Applying database migration rules ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD new file mode 100644 index 000000000..696c9628b --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/CONTRACT_HEAD @@ -0,0 +1 @@ +1b294093239c diff --git a/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD new file mode 100644 index 000000000..e732495bb --- /dev/null +++ b/neutron/db/migration/alembic_migrations/versions/EXPAND_HEAD @@ -0,0 +1 @@ +59cb5b6cf4d diff --git a/neutron/db/migration/cli.py b/neutron/db/migration/cli.py index 7a2fbbeb4..99bf7e605 100644 --- a/neutron/db/migration/cli.py +++ b/neutron/db/migration/cli.py @@ -32,6 +32,8 @@ from neutron.db import migration HEAD_FILENAME = 'HEAD' HEADS_FILENAME = 'HEADS' +CONTRACT_HEAD_FILENAME = 'CONTRACT_HEAD' +EXPAND_HEAD_FILENAME = 'EXPAND_HEAD' CURRENT_RELEASE = migration.MITAKA RELEASES = ( @@ -235,7 +237,6 @@ def _check_bootstrap_new_branch(branch, version_path, addn_kwargs): def do_revision(config, cmd): - '''Generate new revision files, one per branch.''' kwargs = { 'message': CONF.command.message, 'autogenerate': CONF.command.autogenerate, @@ -247,7 +248,10 @@ def do_revision(config, cmd): kwargs['head'] = 'contract@head' do_alembic_command(config, cmd, **kwargs) - update_head_file(config) + if _use_separate_migration_branches(config): + update_head_files(config) + else: + update_head_file(config) def _get_release_labels(labels): @@ -340,12 +344,14 @@ def _get_branch_points(script): def validate_head_file(config): '''Check that HEAD file contains the latest head for the branch.''' if _use_separate_migration_branches(config): - return - _validate_head_file(config) + _validate_head_files(config) + else: + _validate_head_file(config) @debtcollector.removals.remove(message=BRANCHLESS_WARNING) def _validate_head_file(config): + '''Check that HEAD file contains the latest head for the branch.''' script = alembic_script.ScriptDirectory.from_config(config) expected_head = script.get_heads() head_path = _get_head_file_path(config) @@ -361,22 +367,63 @@ def _validate_head_file(config): % expected_head) -def update_head_file(config): - '''Update HEAD file with the latest branch head.''' - if _use_separate_migration_branches(config): - # Kill any HEAD(S) files because we don't rely on them for branch-aware - # chains anymore - files_to_remove = [ - _get_head_file_path(config), _get_heads_file_path(config) - ] - for file_ in files_to_remove: - fileutils.delete_if_exists(file_) +def _get_heads_map(config): + script = alembic_script.ScriptDirectory.from_config(config) + heads = script.get_heads() + head_map = {} + for head in heads: + if CONTRACT_BRANCH in script.get_revision(head).branch_labels: + head_map[CONTRACT_BRANCH] = head + else: + head_map[EXPAND_BRANCH] = head + return head_map + + +def _check_head(branch_name, head_file, head): + try: + with open(head_file) as file_: + observed_head = file_.read().strip() + except IOError: + pass + else: + if observed_head != head: + alembic_util.err( + _('%(branch)s HEAD file does not match migration timeline ' + 'head, expected: %(head)s') % {'branch': branch_name.title(), + 'head': head}) + + +def _validate_head_files(config): + '''Check that HEAD files contain the latest head for the branch.''' + contract_head = _get_contract_head_file_path(config) + expand_head = _get_expand_head_file_path(config) + if not os.path.exists(contract_head) or not os.path.exists(expand_head): + alembic_util.warn(_("Repository does not contain HEAD files for " + "contract and expand branches.")) return - _update_head_file(config) + head_map = _get_heads_map(config) + _check_head(CONTRACT_BRANCH, contract_head, head_map[CONTRACT_BRANCH]) + _check_head(EXPAND_BRANCH, expand_head, head_map[EXPAND_BRANCH]) + + +def update_head_files(config): + '''Update HEAD files with the latest branch heads.''' + head_map = _get_heads_map(config) + contract_head = _get_contract_head_file_path(config) + expand_head = _get_expand_head_file_path(config) + with open(contract_head, 'w+') as f: + f.write(head_map[CONTRACT_BRANCH] + '\n') + with open(expand_head, 'w+') as f: + f.write(head_map[EXPAND_BRANCH] + '\n') + + old_head_file = _get_head_file_path(config) + old_heads_file = _get_heads_file_path(config) + for file_ in (old_head_file, old_heads_file): + fileutils.delete_if_exists(file_) @debtcollector.removals.remove(message=BRANCHLESS_WARNING) -def _update_head_file(config): +def update_head_file(config): script = alembic_script.ScriptDirectory.from_config(config) head = script.get_heads() with open(_get_head_file_path(config), 'w+') as f: @@ -483,6 +530,24 @@ def _get_heads_file_path(config): HEADS_FILENAME) +def _get_contract_head_file_path(config): + ''' + Return the path of the file that is used to maintain contract head + ''' + return os.path.join( + _get_root_versions_dir(config), + CONTRACT_HEAD_FILENAME) + + +def _get_expand_head_file_path(config): + ''' + Return the path of the file that is used to maintain expand head + ''' + return os.path.join( + _get_root_versions_dir(config), + EXPAND_HEAD_FILENAME) + + def _get_version_branch_path(config, release=None, branch=None): version_path = _get_root_versions_dir(config) if branch and release: diff --git a/neutron/tests/unit/db/test_migration.py b/neutron/tests/unit/db/test_migration.py index c57bd3a56..578343154 100644 --- a/neutron/tests/unit/db/test_migration.py +++ b/neutron/tests/unit/db/test_migration.py @@ -24,6 +24,7 @@ from alembic.operations import ops as alembic_ops from alembic import script as alembic_script import fixtures import mock +from oslo_utils import fileutils import pkg_resources import sqlalchemy as sa @@ -118,6 +119,7 @@ class TestCli(base.BaseTestCase): self.do_alembic_cmd_p = mock.patch.object(cli, 'do_alembic_command') self.do_alembic_cmd = self.do_alembic_cmd_p.start() self.mock_alembic_err = mock.patch('alembic.util.err').start() + self.mock_alembic_warn = mock.patch('alembic.util.warn').start() self.mock_alembic_err.side_effect = SystemExit def mocked_root_dir(cfg): @@ -210,7 +212,7 @@ class TestCli(base.BaseTestCase): self.assertEqual(len(self.projects), validate.call_count) def _test_database_sync_revision(self, separate_branches=True): - with mock.patch.object(cli, 'update_head_file') as update,\ + with mock.patch.object(cli, 'update_head_files') as update,\ mock.patch.object(cli, '_use_separate_migration_branches', return_value=separate_branches): if separate_branches: @@ -355,22 +357,61 @@ class TestCli(base.BaseTestCase): def test_upgrade_rejects_delta_with_relative_revision(self, use_mock): self.assert_command_fails(['prog', 'upgrade', '+2', '--delta', '3']) - def _test_validate_head_file_helper(self, heads, file_heads=None, - branchless=False): + def _test_validate_head_file_helper(self, heads, file_heads=None): if file_heads is None: file_heads = [] fake_config = self.configs[0] mock_open = self.useFixture( - tools.OpenFixture(cli._get_head_file_path(fake_config), - '\n'.join(file_heads)) - ).mock_open + tools.OpenFixture(cli._get_head_file_path(fake_config), + '\n'.join(file_heads))).mock_open with mock.patch('alembic.script.ScriptDirectory.from_config') as fc,\ mock.patch.object(cli, '_use_separate_migration_branches', - return_value=not branchless): + return_value=False): + fc.return_value.get_heads.return_value = heads + if all(head in file_heads for head in heads): + cli.validate_head_file(fake_config) + else: + self.assertRaises( + SystemExit, + cli.validate_head_file, + fake_config + ) + self.assertTrue(self.mock_alembic_err.called) + mock_open.assert_called_with( + cli._get_head_file_path(fake_config)) + + fc.assert_called_once_with(fake_config) + + def _test_validate_head_files_helper(self, heads, contract_head='', + expand_head=''): + fake_config = self.configs[0] + head_files_not_exist = (contract_head == expand_head == '') + with mock.patch('alembic.script.ScriptDirectory.from_config') as fc,\ + mock.patch('os.path.exists') as os_mock,\ + mock.patch.object(cli, '_use_separate_migration_branches', + return_value=True): + if head_files_not_exist: + os_mock.return_value = False + else: + os_mock.return_value = True + fc.return_value.get_heads.return_value = heads - if not branchless or all(head in file_heads for head in heads): + revs = {heads[0]: FakeRevision(labels=cli.CONTRACT_BRANCH), + heads[1]: FakeRevision(labels=cli.EXPAND_BRANCH)} + fc.return_value.get_revision.side_effect = revs.__getitem__ + mock_open_con = self.useFixture( + tools.OpenFixture(cli._get_contract_head_file_path( + fake_config), contract_head + '\n')).mock_open + mock_open_ex = self.useFixture( + tools.OpenFixture(cli._get_expand_head_file_path( + fake_config), expand_head + '\n')).mock_open + + if contract_head in heads and expand_head in heads: + cli.validate_head_file(fake_config) + elif head_files_not_exist: cli.validate_head_file(fake_config) + self.assertTrue(self.mock_alembic_warn.called) else: self.assertRaises( SystemExit, @@ -379,35 +420,76 @@ class TestCli(base.BaseTestCase): ) self.assertTrue(self.mock_alembic_err.called) - if branchless: - mock_open.assert_called_with( - cli._get_head_file_path(fake_config)) + if contract_head in heads and expand_head in heads: + mock_open_ex.assert_called_with( + cli._get_expand_head_file_path(fake_config)) + mock_open_con.assert_called_with( + cli._get_contract_head_file_path(fake_config)) + + if not head_files_not_exist: fc.assert_called_once_with(fake_config) - else: - self.assertFalse(mock_open.called) - self.assertFalse(fc.called) - def test_validate_head_file_multiple_heads(self): - self._test_validate_head_file_helper(['a', 'b']) + def test_validate_head_files_success(self): + self._test_validate_head_files_helper(['a', 'b'], contract_head='a', + expand_head='b') - def test_validate_head_file_missing_file(self): - self._test_validate_head_file_helper(['a']) + def test_validate_head_files_missing_file(self): + self._test_validate_head_files_helper(['a', 'b']) + + def test_validate_head_files_wrong_contents(self): + self._test_validate_head_files_helper(['a', 'b'], contract_head='c', + expand_head='d') - def test_validate_head_file_wrong_contents(self): + def test_validate_head_file_branchless_wrong_contents(self): self._test_validate_head_file_helper(['a'], ['b']) - def test_validate_head_file_success(self): + def test_validate_head_file_branchless_success(self): self._test_validate_head_file_helper(['a'], ['a']) - @mock.patch.object(cli, '_use_separate_migration_branches', - return_value=False) - def test_validate_head_file_branchless_failure(self, *args): - self._test_validate_head_file_helper(['a'], ['b'], branchless=True) + def test_validate_head_file_branchless_missing_file(self): + self._test_validate_head_file_helper(['a']) + + def test_update_head_file_success(self): + head = ['b'] + mock_open = self.useFixture( + tools.OpenFixture(cli._get_head_file_path( + self.configs[0]))).mock_open + with mock.patch('alembic.script.ScriptDirectory.from_config') as fc: + fc.return_value.get_heads.return_value = head + cli.update_head_file(self.configs[0]) + mock_open.return_value.write.assert_called_with( + '\n'.join(head)) @mock.patch.object(cli, '_use_separate_migration_branches', - return_value=False) - def test_validate_head_file_branchless_success(self, *args): - self._test_validate_head_file_helper(['a'], ['a'], branchless=True) + return_value=True) + @mock.patch.object(fileutils, 'delete_if_exists') + def test_update_head_files_success(self, *mocks): + heads = ['a', 'b'] + mock_open_con = self.useFixture( + tools.OpenFixture(cli._get_contract_head_file_path( + self.configs[0]))).mock_open + mock_open_ex = self.useFixture( + tools.OpenFixture(cli._get_expand_head_file_path( + self.configs[0]))).mock_open + with mock.patch('alembic.script.ScriptDirectory.from_config') as fc: + fc.return_value.get_heads.return_value = heads + revs = {heads[0]: FakeRevision(labels=cli.CONTRACT_BRANCH), + heads[1]: FakeRevision(labels=cli.EXPAND_BRANCH)} + fc.return_value.get_revision.side_effect = revs.__getitem__ + cli.update_head_files(self.configs[0]) + mock_open_con.return_value.write.assert_called_with( + heads[0] + '\n') + mock_open_ex.return_value.write.assert_called_with(heads[1] + '\n') + + old_head_file = cli._get_head_file_path( + self.configs[0]) + old_heads_file = cli._get_heads_file_path( + self.configs[0]) + delete_if_exists = mocks[0] + self.assertIn(mock.call(old_head_file), + delete_if_exists.call_args_list) + self.assertIn(mock.call(old_heads_file), + delete_if_exists.call_args_list) def test_get_project_base(self): config = alembic_config.Config()