Add run filtering by jakubsacha · Pull Request #73 · trivago/rumi

jakubsacha · 2017-04-04T09:35:03Z

This pull request adds possibility to use --job and --stage options during local rumi execution.

jakubsacha · 2017-04-04T09:42:02Z

ppokalyukhina · 2017-04-04T09:51:40Z

-            /** @var ConfigReader $configReader */
-            $configReader = $this->container->get('trivago.rumi.services.config_reader');
+            /** @var ConfigReaderInterface $configReader */
+            $configReader = $this->container->get('trivago.rumi.services.config_reader_filter_decorator');


Could config_reader_filter_decorator be injected directly here instead of getting a service via container->get ?

ppokalyukhina · 2017-04-04T09:53:01Z

-     * @var ConfigReader
+     * @var ConfigReaderFilterDecorator
     */
    private $configReader;


Shouldn't that be also renamed to $configReaderFilterDecorator?

ppokalyukhina · 2017-04-04T10:06:32Z

+
+            if ($input->getOption(self::STAGE_FILTER)) {
+                $this->configReader->setStageFilter($input->getOption(self::STAGE_FILTER));
+            }


Is there a bit more "elegant" way of setting the filter?

ppokalyukhina · 2017-04-04T10:11:44Z

+    {
+        foreach($this->stages as $k => $s){
+            if ($s === $stage) {
+                unset ($this->stages[$k]);


should be space between unset and brackets maybe removed ?

ppokalyukhina · 2017-04-04T10:20:31Z

+        return $config;
+    }
+
+    public function setStageFilter($stageFilter)


annotation is missing

adriferracuti

Looks good so far, just minor things and a suggestion :)

adriferracuti · 2017-04-05T08:30:23Z

+     */
+    public function remove(JobConfig $job)
+    {
+        foreach ($this->jobs as $k=>$j) {


adriferracuti · 2017-04-05T09:08:31Z

+    }
+
+    /**
+     * @param null $jobFilter


@param string $jobFilter

adriferracuti · 2017-04-05T09:09:07Z

+     */
+    private $configReader;
+    /**
+     * @var null


@var null|string

adriferracuti · 2017-04-05T09:09:13Z

+     */
+    private $stageFilter;
+    /**
+     * @var null


@var null|string

adriferracuti · 2017-04-05T09:25:07Z

+                if (strpos(mb_strtolower($stage->getName()), $this->stageFilter) === false ){
+                    $stagesCollection->remove($stage);
+                }
+            }


You could move this logic to the collection itself, by adding, for example, a filterByStage($stageFilter) method

adriferracuti · 2017-04-05T09:27:20Z

+                if (!count($stage->getJobs()->getIterator())) {
+                    $stagesCollection->remove($stage);
+                }
+            }


You could move this logic to the collection itself, by adding, for example, a removeEmptyStages() method

adriferracuti · 2017-04-05T09:28:13Z

+{
+    /**
+     * @param $workingDir
+     * @param $configFile


types are missing

adriferracuti · 2017-04-05T09:28:48Z

        //given
        $config = [
-            'stage1Name' => []
+            'stage1Name' => [],


intended? :)

Fleshgrinder

Very inconsistent style, and you should avoid mocking in the tests, especially because you never leave architectural boundaries. Test execution would be faster, and it would tell you more about your software.

Mocks are a code smell

— Greg Young

Fleshgrinder · 2017-04-20T06:06:08Z

            $output->writeln('<error>' . $e->getMessage() . '</error>');

-            return -1;
+            return $e->getCode() != 0 ? $e->getCode() : -1;


-1 is not a valid exit code, returning that to the shell is a bad idea: http://www.tldp.org/LDP/abs/html/exitcodes.html

Avoid type unsafe comparison by all means (=== instead of ==).

Use positive conditions, they are generally easier to understand.

This does exactly what you need: 😉

return (int) $e->getCode() !== 0;

Fleshgrinder · 2017-04-20T06:07:08Z

        try {
-            /** @var ConfigReader $configReader */
+            /** @var ConfigReaderInterface $configReader */
            $configReader = $this->container->get('trivago.rumi.services.config_reader');


Avoid this kind of inline IDE comments and install the Symfony extension for PhpStorm instead.

Fleshgrinder · 2017-04-20T06:08:30Z

    {
        try {
-            if (trim($input->getArgument('volume')) != '') {
+            if (trim($input->getArgument('volume')) !== '') {


Doesn't Symfony trim the arguments anyways?

Fleshgrinder · 2017-04-20T06:11:28Z

+            return '';
+        }
+
+        return $this->workingDir . '/';


This working directory code is 1:1 the same as in RunCommand: trait?

Fleshgrinder · 2017-04-20T06:14:08Z

+            return '';
+        }
+        return (string)$this->input->getOption(RunCommand::JOB_FILTER);
+    }


I think it makes sense to create your own extended input instead of using so many procedural constants. It would also allow you to decouple yourself from the Symfony console component, and it would make your code much simpler.

Fleshgrinder · 2017-04-20T06:15:06Z

+
+interface JobFilterParametersInterface
+{
+    public function getJobFilter(): string;


public in an interface is plain wrong, it cannot be anything else.

Fleshgrinder · 2017-04-20T06:16:17Z

+
+        $this->container->set('trivago.rumi.services.config_reader', $this->configReader->reveal());

        $this->SUT = new CacheStoreCommand(


Your style is extremely inconsistent: all upper prop, really?

Fleshgrinder · 2017-04-20T06:17:25Z

    public function testGivenCiFileDoesNotExist_WhenCacheStoreRun_ThenItSkipsExecution()
    {
        // given
+        $this->configReader->getRunConfig()->willThrow(new \RuntimeException('', ReturnCodes::RUMI_YML_DOES_NOT_EXIST));


Create your own exception instead of using procedural constants.

jakubsacha requested review from adriferracuti, hendrikmaus and ppokalyukhina April 4, 2017 09:35

hendrikmaus approved these changes Apr 4, 2017

View reviewed changes

ppokalyukhina reviewed Apr 4, 2017

View reviewed changes

adriferracuti suggested changes Apr 5, 2017

View reviewed changes

Fleshgrinder suggested changes Apr 20, 2017

View reviewed changes

Fleshgrinder added the enhancement label Apr 20, 2017

jakubsacha added 6 commits May 10, 2017 15:50

Add support for --job and --stage for local executions

b13aa88

Finalise implementation of job and stage filtering

9ad4648

Adjust Readme.md

260665a

Adjust changelog

bbe267f

Some refactoring...

b9449c4

Code style fixes

870074e

jakubsacha force-pushed the add-run-filtering branch from ee173e4 to 870074e Compare May 10, 2017 13:50


		$this->container->set('trivago.rumi.services.config_reader', $this->configReader->reveal());

		$this->SUT = new CacheStoreCommand(

Conversation

jakubsacha commented Apr 4, 2017

Uh oh!

jakubsacha commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriferracuti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fleshgrinder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fleshgrinder left a comment •

edited

Loading