Skip to content
This repository has been archived by the owner on May 30, 2022. It is now read-only.

Add cluster settings api #546

Merged

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Dec 7, 2021

This PR exposes an api at GET /api/clusters/settings to retrieve the clusters settings for the runner to provide to Ansible.

Involved settings are:

  • selected checks for a cluster
  • hosts for a given cluster with address and user to enable ansible connection

Example API response.

[
    {
        "id": "c115e7ecf5901d9a768d06a28ab0ba26",
        "selected_checks": [],
        "hosts": [
            {
                "name": "vmnetweaver01",
                "address": "x.x.x.x",
                "user": "root"
            },
            {
                "name": "vmnetweaver02",
                "address": "x.x.x.x",
                "user": "root"
            }
        ]
    },
    {
        "id": "5dfbd28f35cbfb38969f9b99243ae8d4",
        "selected_checks": [
            "A",
            "B",
            "C",
        ],
        "hosts": [
            {
                "name": "vmhana01",
                "address": "x.x.x.x",
                "user": "cloudadmin"
            },
            {
                "name": "vmhana02",
                "address": "x.x.x.x",
                "user": "cloudadmin"
            }
        ]
    }
]

This allows the runner to only call one API and provided useful information to Ansible and remove consul dependency from the runner itself (next PR).

cmd/agent/agent.go Outdated Show resolved Hide resolved
web/app.go Outdated Show resolved Hide resolved
web/models/clusters_settings.go Outdated Show resolved Hide resolved
web/services/clusters.go Outdated Show resolved Hide resolved
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

I have some hard feelings about did.
We already did a huge effort creating the "/checks/:id/settings" group of endpoint for this exact purpose.
This PR in my opinion totally overlaps this.
Did we consider simply adding the connection data information to the currently existing endpoints?
In essence, the information used in the runner, and shown in the frontend is exactly the same

@@ -186,6 +186,7 @@ func NewAppWithDeps(config *Config, deps Dependencies) (*App, error) {
apiGroup.POST("/clusters/:id/tags", ApiClusterCreateTagHandler(deps.clustersService, deps.tagsService))
apiGroup.DELETE("/clusters/:id/tags/:tag", ApiClusterDeleteTagHandler(deps.clustersService, deps.tagsService))
apiGroup.GET("/clusters/:cluster_id/results", ApiClusterCheckResultsHandler(deps.consul, deps.checksService))
apiGroup.GET("/clusters/settings", ApiGetClustersSettingsHandler(deps.clustersService))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm super confused with this.
We already have the "/checks/:id/settings" end point for this. Where the id is the ansible group id (as we need to group the ansible checks by group0.
This was named on purpose this way, as we were already seeing high changes to have other checks groups (like SAP system wide checks, or SAP landscapes wide checks)

@nelsonkopliku nelsonkopliku force-pushed the add_cluster_settings_api branch 2 times, most recently from cf2e948 to 116abd0 Compare December 13, 2021 09:29
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Mega-LGTM

@nelsonkopliku nelsonkopliku dismissed stefanotorresi’s stale review December 13, 2021 11:09

All the changes have been addressed already and there's team consensous on those

@nelsonkopliku nelsonkopliku merged commit 17cd4fe into trento-project:main Dec 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Wrap all the GetSelectedChecksById calls in the runner, to a unique API call
5 participants