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

[shopsys] standards for .yaml and yaml.dist files #539

Merged
merged 6 commits into from Jul 29, 2019

Conversation

@sspooky13
Copy link
Contributor

sspooky13 commented Oct 22, 2018

Q A
Description, reason for the PR add yaml standards into project for better and cleaner yaml files
New feature Yes
BC breaks No
Fixes issues resolves #666, closes #1117
Standards and tests pass Yes
Have you read and signed our License Agreement for contributions? Yes

Hello, as i promised, i finished my yaml standards library https://github.com/sspooky13/yaml-standards. I try in short explain what that library can do.
As first arguments are sent path to yaml (or dist) file or you can sent only path to directory and library find all yaml, yml, yml.dist, yaml.dist files recursive. You must send at least one path.
Next, you can send option for files names which ones want to exclude from check, e.g. --exclude=service and now all files have in name 'service' text is excluded.
Next options are for checks, i can try to explain their:
Alphabetical checker: check files are alphabetically sort. Simply add to call command option --check-alphabetical-sort-depth=2. With this option you enable check alphabetical sort and you must set a depth for check. I explain the depth in the code in bottom.

firstDepth:
   secondDepth:
       thirdDepth: etc.

Indent checker: check correct indents in line. You call command with option --check-indents-count-of-indents=4 and file must have multiples of four indents. I warn, the note must have same coun of indent as text bottom of him. I will give an example:

#bad indents
firstParent:
  secondParent:
#bad note
    thirdParent: etc.
#correct indents
firstParent:
    secondParent:
        #correct note
        thirdParent: value

Space between groups checker: check space (or empty line) between groups. You call command with option --check-spaces-between-groups-to-level=2 and now file must have empty line before start of groups to level 2. Example

firstParent:
    #first of element must not have a empty line
    secondParent:
        key: value
        key2: value
        key3: value

    #here must be a empty line before a parent and his notes
    secondParent2:
        key: value
        key2: value
        key3: value

Observance style (inline) checker: this is simple check observance style of yaml file from symfony. You call command with option --check-inline and file must observe style of code as return from Yaml::dump() with set 3 inline parameter. Example:

Shopsys\ShopBundle\Component\Javascript\Compiler\JsCompiler:
    arguments:
      -
        - '@Shopsys\ShopBundle\Component\Javascript\Compiler\Translator\JsTranslatorCompilerPass'
        - '@Shopsys\ShopBundle\Component\Javascript\Compiler\Constant\JsConstantCompilerPass'

Shopsys\ShopBundle\Component\Javascript\Compiler\JsCompiler:
    arguments:
        - ['@Shopsys\ShopBundle\Component\Javascript\Compiler\Translator\JsTranslatorCompilerPass', '@Shopsys\ShopBundle\Component\Javascript\Compiler\Constant\JsConstantCompilerPass']

For a sample, i add alphabetical checker, space between groups checker and indent checker for service.yml and i fixed it by standards. In the beginning, this checks without fixers are annoying and slow but after finish, you can see that heartwarming result :)
This library has not yet fixxer but in future will be. I will be glad for every feedback.

Thanks you approve this.

@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Oct 25, 2018

The library only supports checking at the moment, so there is no automated way to fix the violations, which may render the usage problematic (especially the alphabetic sorting). It is configured to only check a single file (it may be configured to check directories recursively though).

I would like to find some time to implement the alphabetic fixer as I have some ideas how to get it working pretty easily. I've already talked about it with @sspooky13 and we've agreed to cooperate on this, so the tools should be compatible.

I would not recommend to use it as-is to check the alphabetical order, but it might be a great idea to implement other checks (such as correct indentation) for all yaml configs in the repository.

Nice work, @sspooky13, on the library 👍

@Miroslav-Stopka

This comment has been minimized.

Copy link
Contributor

Miroslav-Stopka commented Oct 29, 2018

Hi @sspooky13,

thank you for this really handy tool :). As Peter already said, in current form, we will probably use it just for indentation checks. What do you say?

Can you modify this PR in such a way:

  • the first commit contains only adding this tool
  • the second commit contains all modified (checked) yml files

There is also a variant that I will modify this PR if it suits you more.

@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Oct 30, 2018

Hello @Miroslav-Stopka,

i try modify PR today, at the latest the end of week i do it.

@Miroslav-Stopka

This comment has been minimized.

Copy link
Contributor

Miroslav-Stopka commented Oct 30, 2018

@sspooky13, youre the best :)

I think, after this PR will be merged, some 🍻 will be prepared here for you.

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from 3e565bd to 19f92e1 Nov 2, 2018
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Nov 5, 2018

@Miroslav-Stopka, i apologize but i dont have a time prev week. I think at the latest the end of week i make it done but i dont promise yet 😀

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from 19f92e1 to 6492de7 Nov 9, 2018
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Nov 11, 2018

Hello @Miroslav-Stopka, i try to fix indent checker because i did not think about a array with key and value in same line, e.g.

foo:
    - foo: bar
      baz: qux
      etc.: etc.

and it's more difficult as i did think. I try to fix it 2 week but i do not have it complete.
Will it be ok for you when I promise to try to deliver it as soon as possible?

Thank you for your understanding.

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from 6492de7 to 8b8391b Nov 11, 2018
@Miroslav-Stopka

This comment has been minimized.

Copy link
Contributor

Miroslav-Stopka commented Nov 12, 2018

Hi @sspooky13,
sure thing :)

@boris-brtan

This comment has been minimized.

Copy link
Contributor

boris-brtan commented Nov 16, 2018

+1

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch 2 times, most recently from 64dfe55 to b872c59 Nov 17, 2018
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Nov 17, 2018

Hello everyone,

i finally fix my indent checker and i finished fixing yaml files in projects.
Except for one file, ./packages/coding-standards/easy-coding-standard.yml, where i have a problem with last comment. I add right indent for comment by him next key. If comment is in end of file then comment has zero indents. I think it makes sense when comment point to line bottom of him, not up to him. As you can see i change a few file where i moved the commend on the line above. For this file i dont know where a i move this commend because i dont like it somewhere. Can anyone help me with this?

Because i use symfony yaml parser before i run checks i must edit keys which start with [ and values have in next lines because symfony parser dont support this style of array. I change it to style with dash which should be correct alternative. I hope its ok.

At last i extend indent checker about a check correct indent between dash and text, e.g.

- '*/src/Shopsys/ShopBundle/Controller/Front/OrderController.php'
-   name: footer
     width: 1160
     height: ~

Arrays with one value (first line in the code above) must have only one indent between dash and text.
Multi values array (others line in the code above) must have indent by set variable minus one indent because the dash is taken as indent. Its ok for you?

P.S.: i understand @PetrHeinz idea about alphabetical checker is not good without fixer but you must not add the check for all yaml files, you can add it to for a few where it does not matter and then you can add it to next files but its yours choice.

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from b872c59 to 3f834a1 Nov 19, 2018
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Nov 25, 2018

Hello @PetrHeinz, i fixed your points a few days ago. I understand maybe you dont have a time but i dont know whether github send email about my reaction, so i write next message for sure.

Copy link
Contributor

boris-brtan left a comment

Hi @sspooky13, THX for this super cool tool, i think that after minor fix we need to find the right place for this tool in our monorepo.

microservices/product-search/config/services.yaml Outdated Show resolved Hide resolved
microservices/product-search/config/services.yaml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from 3f834a1 to 56826eb Dec 9, 2018
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Dec 9, 2018

Hello @boris-brtan, i fixed up mentioned errors. Can you check it again, whether is already alright?

Thanks for your patience.

@boris-brtan

This comment has been minimized.

Copy link
Contributor

boris-brtan commented Dec 11, 2018

thx for the fixes, now we need to find the right place of this tool in monorepo so also project-base would be able to use it or anyone who will use shopsys/coding-standards package

@boris-brtan

This comment has been minimized.

Copy link
Contributor

boris-brtan commented Jan 9, 2019

hi @sspooky13 , there is some problem with IteratorIterator that tries to access project-base/var/postgres-data folder that is inaccessible due to user rights can you reimplement excludeFileMasks somehow like this? $this->isReadable() should secure that it will not try to access inaccessible folders and files

# YamlFilesPathService.php
<?php

namespace YamlStandards\Service;

use RecursiveDirectoryIterator;
use RecursiveFilterIterator;
use RecursiveIteratorIterator;
use RecursiveRegexIterator;
use RegexIterator;

class YamlFilesPathService
{
    /**
     * @param string[] $pathToDirsOrFiles
     * @param string[] $excludedFileMasks
     * @return string[]
     */
    public static function getPathToYamlFiles(array $pathToDirsOrFiles, array $excludedFileMasks)
    {
        $pathToFiles = [];
        foreach ($pathToDirsOrFiles as $pathToDirOrFile) {
            if (is_file($pathToDirOrFile)) {
                $pathToFiles[] = $pathToDirOrFile;
                continue;
            }

            $recursiveDirectoryIterator = new RecursiveDirectoryIterator($pathToDirOrFile);
            $recursiveIteratorIterator = new RecursiveIteratorIterator(new class($recursiveDirectoryIterator, $excludedFileMasks) extends RecursiveFilterIterator {
                private $excludedFileMasks;
                public function __construct($iterator, array $excludedFileMasks)
                {
                    parent::__construct($iterator);
                    $this->excludedFileMasks = $excludedFileMasks;
                }
                public function accept()
                {
                    return $this->isReadable() && count(preg_grep('#'. join('|',$this->excludedFileMasks) .'#i', [$this->getPathName()])) === 0;
                }
                public function getChildren()
                {
                    return new self($this->getInnerIterator()->getChildren(), $this->excludedFileMasks);
                }
            });
            $regexIterator = new RegexIterator($recursiveIteratorIterator, '/^.+\.(yml|yaml|yaml\.dist|yml\.dist)$/i', RecursiveRegexIterator::GET_MATCH);

            foreach ($regexIterator as $pathToFile) {
                $pathToFiles[] = reset($pathToFile);
            }
            var_dump($pathToFiles);
        }

        $pathToFiles = str_replace('\\', '/', $pathToFiles);

        return array_unique($pathToFiles);
    }
}

after that we will be able to launch it with this command
./vendor/bin/yaml-standards --exclude=project-base/var/postgres-data --exclude=vendor --exclude=node_modules --check-indents-count-of-indents =4 ./

@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Jan 16, 2019

hello @boris-brtan, thanks for you "pull request" or "creating issue". I look it during weekend and either i change the class by your request or i give a comment here.

@sspooky13 sspooky13 force-pushed the sspooky13:pt-yaml-standards branch from 56826eb to ee9f8d0 Jan 21, 2019
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Jan 21, 2019

hello @boris-brtan, as first i must say your example is bad because https://github.com/sspooky13/yaml-standards support also PHP version 5.6 and in this version anonymous classes not exist + even if support it i don't use them because it causes complexity of code.
BTW: i created next option exclude-dir, you can write --exclude-dir=vendor and now checker ignore vendor in root directory
PS: you must write full path to excluded dir

In future, i will be glad you create up issue into repository than writing ideas into your issues

Copy link
Contributor

boris-brtan left a comment

It looks much better than before, i have just some suggestions that will provide yaml-standards not just for monorepo but also for project-base developers.
Some Changes for adding yaml standards are in yaml: fix all bad indents and according to symfony yaml bad code but i think sqash and merge will solve it very fast

build.xml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
build.xml Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
project-base/build.xml Outdated Show resolved Hide resolved
@PetrHeinz PetrHeinz closed this Jul 25, 2019
@PetrHeinz PetrHeinz reopened this Jul 25, 2019
@PetrHeinz PetrHeinz requested a review from TomasLudvik Jul 25, 2019
@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Jul 25, 2019

It seems that the changes it did to project-base/kubernetes/kustomize/overlays/ci/kustomization.yaml are not equivalent:

  patchesJson6902:
- -   target:
+     - target:
          group: extensions
          version: v1beta1
          kind: Ingress
          name: shopsys
-     path: ./ingress-patch.yaml
+         path: ./ingress-patch.yaml
  configMapGenerator:

The patchesJson6902 originally hold this value:

  [
    {
      "target": {
        "group": "extensions", 
        "version": "v1beta1", 
        "kind": "Ingress", 
        "name": "shopsys"
      },
      "path": "./ingress-patch.yaml", 
    }
  ]

But it was changed to this:

  [
    {
      "target": {
        "group": "extensions", 
        "version": "v1beta1", 
        "kind": "Ingress", 
        "name": "shopsys",
        "path": "./ingress-patch.yaml"
      }
    }
  ]
Copy link
Member

TomasLudvik left a comment

I have watched it two times and only problem I have found was same as found @PetrHeinz in #539 (comment)

@PetrHeinz PetrHeinz changed the title standards for .yaml and yaml.dist files [shopsys] standards for .yaml and yaml.dist files Jul 25, 2019
@PetrHeinz PetrHeinz force-pushed the sspooky13:pt-yaml-standards branch from 8fc8b48 to a27e94f Jul 25, 2019
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Jul 25, 2019

Hello guys,
I didn't know that this yaml write style is possible so I didn't resolve it 😄.
But if I check this yaml

patchesJson6902:
-   target:
        group: extensions
        version: v1beta1
        kind: Ingress
        name: shopsys
    path: ./ingress-patch.yaml

result of it is

array:1 [
  "patchesJson6902" => array:1 [
    0 => array:2 [
      "target" => array:4 [
        "group" => "extensions"
        "version" => "v1beta1"
        "kind" => "Ingress"
        "name" => "shopsys"
      ]
      "path" => "./ingress-patch.yaml"
    ]
  ]
]

Buf if you remove that dash - you get

patchesJson6902:
    target:
        group: extensions
        version: v1beta1
        kind: Ingress
        name: shopsys
    path: ./ingress-patch.yaml

and result it

array:1 [
  "patchesJson6902" => array:2 [
    "target" => array:4 [
      "group" => "extensions"
      "version" => "v1beta1"
      "kind" => "Ingress"
      "name" => "shopsys"
    ]
    "path" => "./ingress-patch.yaml"
  ]
]

and this style works with my checker/fixer and it seems better to me but I understand I have a bug in my library.
If you can change style of code you can remove that dash and the all will be ok. If you can not change style then you must wait for fix this bug but I don't have time in next days/weeks ☹
Thanks for report that bug, I am glad to see any progress in this PR 👍.

PetrHeinz added a commit to PetrHeinz/yaml-standards that referenced this pull request Jul 26, 2019
@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Jul 26, 2019

Seeing that there might be other bugs apart from sspooky13/yaml-standards#26 when run on a more exotic YAML syntax, I have made the dependency optional in b6dd3e0. Now, the whole YAML standards checking can be abandoned by just running composer remove sspooky13/yaml-standards --dev on a project if any problems arise.

Also it means easier upgrading 🙂

@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Jul 26, 2019

@TomasLudvik The bugfix is ready in sspooky13/yaml-standards#27, so now we wait for review, testing, and hopefully merging and releasing v4.2.4 of sspooky13/yaml-standards by @sspooky13.

Then I'll rebase the PR and drop the HOTFIX ... commit and we can continue with this PR 🙂

If you want to review / test it in the meantime, you can switch the Composer dependency to my fork:

       "packages/framework/src/Component/VarDumper/functions.php"
     ]
   },
+  "repositories": [
+    {
+      "type": "vcs",
+      "url": "https://github.com/PetrHeinz/yaml-standards"
+    }
+  ],
   "autoload-dev": {
     "psr-4": {
       "Tests\\": "project-base/tests/",
     "symplify/monorepo-builder": "^5.2.17",
     "phpstan/phpstan-doctrine": "^0.11.2",
     "phpstan/phpstan-phpunit": "^0.11.2",
-    "sspooky13/yaml-standards": "^4.2"
+    "sspooky13/yaml-standards": "dev-ph-fix-nested-hierarchy"
   },
   "conflict": {
     "symfony/dependency-injection": "3.4.15|3.4.16",
@sspooky13

This comment has been minimized.

Copy link
Contributor Author

sspooky13 commented Jul 26, 2019

Hello,
again, thanks @PetrHeinz for PR.
I merged it to master and release new version 4.2.4. 🎉
Please, do more these PR. It's was very gratifying. 😄

@PetrHeinz PetrHeinz force-pushed the sspooky13:pt-yaml-standards branch from b6dd3e0 to 620dd6c Jul 26, 2019
@PetrHeinz

This comment has been minimized.

Copy link
Contributor

PetrHeinz commented Jul 26, 2019

Rebased after the release of v4.2.4 patch version.

@PetrHeinz PetrHeinz requested a review from TomasLudvik Jul 26, 2019
@PetrHeinz PetrHeinz force-pushed the sspooky13:pt-yaml-standards branch from 3e89cc0 to 1e8e374 Jul 29, 2019
sspooky13 and others added 6 commits Jul 25, 2019
- uses sspooky13/yaml-standards
- targets "yaml-standards" and "yaml-standards-fix" check/fix YAML standards
- target "yaml-standards-availability" checks the availability of the library
    - this is important during the first run as it tells the user what to do
    - having "sspooky13/yaml-standards" only as a --dev dependency in project repository avoids installing itin production environment
- yq uses indentation of 2 spaces instead of 4 spaces that we use
- the indentation cannot be configured (see mikefarah/yq#129)
- the required change is easy enough so that it can be done via sed
@PetrHeinz PetrHeinz force-pushed the sspooky13:pt-yaml-standards branch from 1e8e374 to 6ba40c1 Jul 29, 2019
@PetrHeinz PetrHeinz merged commit e5f9f1b into shopsys:master Jul 29, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@vitek-rostislav

This comment has been minimized.

Copy link
Contributor

vitek-rostislav commented Jul 29, 2019

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.