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

Get catalog from runner #257

Merged
merged 17 commits into from
Mar 31, 2022
Merged

Get catalog from runner #257

merged 17 commits into from
Mar 31, 2022

Conversation

arbulu89
Copy link
Contributor

Implement the backend part of the runner connection to get the catalog.
It includes:

  • 2 adapters to get the catalog. The MockRunner, so we don't need a real runner to create the catalog, and the real Runner, which interacts with the runner. The implement an adapter, so we can use one or another, and even make the testing easier.
  • Configuration of the adapter and the RUNNER_URL flag
  • Update the catalog.json to use the new flat list

TODO next:

  • Update the frontend to do some error handling and call the new api correctly

@arbulu89 arbulu89 force-pushed the get-catalog-from-runner branch from 25561cc to 432620a Compare March 29, 2022 11:24
@arbulu89 arbulu89 force-pushed the get-catalog-from-runner branch from a721e42 to 5c8fb5b Compare March 29, 2022 12:13
config/dev.exs Outdated Show resolved Hide resolved
config/prod.exs Outdated Show resolved Hide resolved
config/runtime.exs Outdated Show resolved Hide resolved
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Nice! I left a couple of comments but @fabriziosestito can help us better 😄

Then a little reasoning.

This guy exposes an endpoint /api/checks/catalog which actually calls /api/catalog on the runner,

Sort of a proxy, but not even, because it does something, namely grouping a flatted catalog received from the Runner, so we could say this is sort of a BFF feature.

runner response

[
  {
    "id": "156F64",
    "name": "1.1.1",
    "group": "Corosync",
    "provider": "azure",
    "description": "...",
    "labels": "generic"
  },
  {
    "id": "156F64",
    "name": "1.1.1",
    "group": "Corosync",
    "provider": "aws",
    "description": "...",
    "labels": "generic"
  },
  ...
]

web response

[
    {
      "groups": [
        {
          "checks": [
            {
                ...
            }
          ],
          "group": "Corosync"
        },
        {
          "checks": [
            {
              ...
            }
          ],
          "group": "Miscellaneous"
        }
      ],
      "provider": "aws"
    },
    {
      "groups": [
        {
          "checks": [
            {
             ...
            }
          ],
          "group": "Corosync"
        },
        {
          "checks": [
            {
              ...
            }
          ],
          "group": "Miscellaneous"
        }
      ],
      "provider": "azure"
    }
  ]

This is perfectly fine.

Yet I'd consider the opportunity to actually just have a proxy for the time being (an ingress or a proxy controller on web).

Why did we chose to do it this way? Mainly to make the grouping on elixir side I believe.

I believe it can go this way.

lib/trento/application/integration/checks/checks.ex Outdated Show resolved Hide resolved
config/dev.exs Outdated Show resolved Hide resolved
@arbulu89 arbulu89 force-pushed the get-catalog-from-runner branch from 7b9d113 to a5d179f Compare March 30, 2022 12:19
@arbulu89
Copy link
Contributor Author

@fabriziosestito @nelsonkopliku ,
Most of your comments (or all of them) addressed in the last commit: a5d179f

Besides that, I have updated slightly the catalog controller to allow a flat param (api/checks/catalog?flat) which will return the flat version of the catalog.

@arbulu89 arbulu89 force-pushed the get-catalog-from-runner branch from a5d179f to 5fe0378 Compare March 30, 2022 12:35
@arbulu89
Copy link
Contributor Author

Why did we chose to do it this way? Mainly to make the grouping on elixir side I believe.

I believe it can go this way.

Yes, mostly the idea is to keep the runner as dump as possible, mostly, because doing list operations in golang is more costly (and much more in ansible...).
Finally, using a simple ingress wouldn't be that easy, because the runner has some certain intelligence, like some state where the catalog is not ready yet. Finally, I took the chance of doing some error handling and messages customization. In any case, the catalog controller is really simple at this point, so I hope it doesn't require too much maintenance.

config/dev.exs Outdated Show resolved Hide resolved
lib/trento/application/integration/checks/checks.ex Outdated Show resolved Hide resolved
lib/trento/application/integration/checks/checks.ex Outdated Show resolved Hide resolved
@fabriziosestito
Copy link
Member

@arbulu89 left some commends about styles / error handling in general.
Some doubts about the naming of the DTOs / models but we can iterate on it

@arbulu89
Copy link
Contributor Author

@fabriziosestito yet another round of changes hehe
I have removed the "stringtified" error handling and enforced the usage of FlatCatalog from the adapter

@arbulu89 arbulu89 merged commit f4adf65 into main Mar 31, 2022
@arbulu89 arbulu89 deleted the get-catalog-from-runner branch March 31, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants