#355 add `config_opts['package_manager']` to cli mock-config command
Closed by julian8628. Opened by julian8628.
julian8628/koji mock-config  into  master

Download 355.patch
  • add config_opts['package_manager'] in cli mock-config
  • change file writing from file() to open() in cli mock-config
  • change print to parser.error in cli mock-config
  • refactor some cli anon_handler_mock_config code
  • unittest for cli mock-config
  • refactor some code in BuildRoot

it's a short-term fix for issue#339

We can't use with syntax until we drop EL5 support

@mikem Unless you intend to make a Koji release before the end of EPEL 5 this month, I think it's pretty safe to drop EL5 support.

We still have some RHEL5 builders internally, so no, it is not safe

3 new commits added

  • format code by autopep8
  • mock-config: unittest for no arg
  • rollback with syntax to support python 2.4

@mikem updated

1 new commit added

  • fix typo

@julian8628 Please rebase against master.

rebased

@ngompa done :)

Why move this code into koji.gen_mock_opts() if we're not going to use it anywhere else?

That said, I'm not sure that code feels right in the library yet. It seems to specific to the BuildRoot class internals.

Let me rollback this code.
I'm supposed to use it in cli.anon_handle_mock_config(), in order to unify mock config generation for both cli and buildroot. But it looks much more modification required.
We could consider it again in long-term solution in future.
BTW I guess put all options generation in one place is good for code maintenance in this case, like MOCK_OPTS_MAPPING

1 new commit added

  • rollback BuildRoot._writeMockConfig

1 new commit added

  • remove empty line

rebased onto f50419eca7716dc240a168a0e53719dcd7f09ba1

rebased onto 45c6f5c6ced4d6961cd9cc66c2751537e3ddc224

This shows as recently rebased, but the rebase is against a fairly old commit. I'm guessing this rebase was actually done a while back and only recently pushed. Or was this intentional?

Overall the current code changes in this patch look ok. However, it will take some work to rebase against current master. Part of the problem are the intermediate changes in the earlier commits. The net change is much more contained. Beyond that, both sides have added a test_mock_config test, which did not exist before.

If we want to get this merged, I'd suggest squashing the commits to ease the rebase.

I don't think there's any easy way to deal with test_mock_config. That will have to involve some manual work. I would suggest starting by initially keeping the existing test from current master. It would be interesting to know how (or if) that test breaks with these changes. Then, you could incorporate any needed adjustments to the test from this earlier work.

will update

rebased onto 428838650a2237af33edb054830302902a676ab7

@mikem I've merged the commits into one and modified the test cases
please review again

Pull-Request has been closed by julian8628

Metadata