Allow multiple SAP system tags per host #160

rtorrero · 2021-07-21T12:58:23Z

This PR adds support for having multiple SIDs on the same host (useful in cost-optimized scenario). The host metadata field is now trento-sap-systems and the filters have been updated to allow matching e.g. PRD in a host where PRD,QAS is stored in the metadata.

Templates have been updated too to now show multiple SIDs where relevant.

Updated: Screenshots

arbulu89 · 2021-07-21T14:57:05Z

@rtorrero I would appreciate some screenshots of the changed templates of the frontend

rtorrero · 2021-07-21T15:02:14Z

@rtorrero I would appreciate some screenshots of the changed templates of the frontend

yes! I wanted to post some but my test environment is broken right now. Deploying a new one and should be able to add them soon

arbulu89

Hi @rtorrero ,
Many comments hehe
In general the implementation looks good, but I think we can remove and re-order many of the new changes.
Finally, as added in independent comment, it would be nice to have a screenshot/gif of the final result

arbulu89 · 2021-07-21T14:58:59Z

agent/discovery/sapsystem.go

@@ -33,33 +33,49 @@ func (d SAPSystemsDiscovery) Discover() error {
 		return err
 	}

+	sapSystems := make([]*sapsystem.SAPSystem, len(systems))


Do we really need a new variable sapSystems to store this information?
This looks like a replica of the systems/d.SAPSystems variable just changing the map to list format

(systems is of sapsystem.SAPSystemsList, which is already a list)

arbulu89 · 2021-07-21T15:00:44Z

agent/discovery/sapsystem.go

-	envName, landName, sysName, err := loadSAPSystemTags(client, system.SID)
-	if err != nil {
-		return err
+func storeSAPSystemTags(client consul.Client, systems []*sapsystem.SAPSystem) error {


As commented above, I think we could use sapsystem.SAPSystemsList as systems type

arbulu89 · 2021-07-21T15:05:43Z

agent/discovery/sapsystem.go

+	var envName, landName string
+	sysNames := ""
+	var err error
+	for i, system := range systems {


This looks a bit over complicated.
I suggest the next:
As envName and landName will be the same independtly which system.SID we use, we could just call it once with the 1st SID.
After that, i would prefer the usage of strings.Join to create the sids string. I don't know if we could use some kind of lamdba method to get the SID, otherwise we will need to loop though all the systems, create a SID string array, and apply the Join function

agent/discovery/sapsystem.go

internal/hosts/hosts.go

arbulu89 · 2021-07-21T15:10:39Z

internal/hosts/hosts.go

+		return nil, err
+	}
+
+	systemsList := make(sapsystem.SAPSystemsList, len(systems))


Why do we have this loop? Cannot we use directly the systems map? At the end they have the same content

I assume that by "using directly the systems map you mean using the GetSAPSystems method?
The thing is that if I use GetSAPSystems in the template, I can't easily add a comma after each SID but the last one (since in a string map, each key is not an int but a string, so I can't easily figure out the len-1 case :/... It might be that I'm not familiar enough with templates. A bit more details if you can think of a way of using the string map in the template would help

As we just need to split the string by the ,, why don't we just add the Split method to our templating system, so we can use it?
Here an example: https://stackoverflow.com/questions/19771787/golang-how-to-split-string-in-template
We would need to add the func here: https://github.com/trento-project/trento/blob/main/web/layout.go#L142

I see that you need this already 2 times, so it would make sense

internal/hosts/hosts.go

arbulu89

@rtorrero Some new comments

internal/sapsystem/sapsystem.go

arbulu89 · 2021-07-23T06:38:08Z

internal/hosts/hosts.go

+		return nil, err
+	}
+
+	systemsList := make(sapsystem.SAPSystemsList, len(systems))


As we just need to split the string by the ,, why don't we just add the Split method to our templating system, so we can use it?
Here an example: https://stackoverflow.com/questions/19771787/golang-how-to-split-string-in-template
We would need to add the func here: https://github.com/trento-project/trento/blob/main/web/layout.go#L142

I see that you need this already 2 times, so it would make sense

rtorrero · 2021-07-26T19:02:30Z

I think I've addressed all pending conversations; but be sure to take another look please @arbulu89

I'm aware of the repetition of code that happens between the two GetSIDsString methods in SAPSystemsList and SAPSystemsMap. I explored the possibility of using reflection to avoid this but in the end, the result is probably worse in the sense that it makes the code way less readable and doesn't save too many lines anyways. Check it out here in any case if you have doubts about it.

I've also added the missing screenshots now that my test env is working fine again

arbulu89

Hi @rtorrero ,
More comments

arbulu89 · 2021-07-27T07:04:18Z

agent/discovery/sapsystem.go

-func storeSAPSystemTags(client consul.Client, system *sapsystem.SAPSystem) error {
-	envName, landName, sysName, err := loadSAPSystemTags(client, system.SID)
+func storeSAPSystemTags(client consul.Client, systems sapsystem.SAPSystemsList) error {
+	envName, landName, _, err := loadSAPSystemTags(client, systems[0].SID)


The loadSAPSystemTags is only used here, so we could even remove this 3rd return value (which is not used)

arbulu89 · 2021-07-27T07:07:26Z

internal/environments/environments.go

@@ -107,6 +107,12 @@ func (l *Landscape) AddSystem(system *SAPSystem) {
 	l.SAPSystems[system.Name] = system
 }

+func (l *Landscape) AddSystems(systems []*SAPSystem) {


This new method is not used anywhere. I think you created this to replace the loop used above maybe, at line 62 of agent/discovery/sapsystem.go.
You could whether remove this function or use it above replacing the loop (you might need to change the incoming variable type)

arbulu89 · 2021-07-27T07:11:41Z

web/layout.go

@@ -149,6 +150,9 @@ func (r *LayoutRender) addFileFromFS(templatesFS fs.FS, file string) {
 			return a + b
 		},
 		"markdown": markdownToHTML,
+		"split": func(s string, sep string) []string {


Did you try just with "split": strings.Split here?

arbulu89 · 2021-07-27T07:15:37Z

web/templates/host.html.tmpl

-            <dd class="inline"><a
-                        href="/sapsystems/{{ $SAPSys }}?environment={{ $Env }}&landscape={{ $Land }}">{{ $SAPSys }}</a>
+            <dd class="inline">
+                {{- range $i, $v := split (.Host).GetSIDsString ","}}{{- if $i }},{{- end }} <a


Do we need this .Host.GetSIDsSting function?
It will return the same value we have in the metadata, right?
I mean, why simply not: split (index .Host.TrentoMeta "trento-sap-systems) ","
And we get rid off GetSIDsString function from the Host struct

arbulu89 · 2021-07-27T07:23:33Z

internal/sapsystem/sapsystem.go

@@ -77,6 +78,26 @@ type CustomCommand func(name string, arg ...string) *exec.Cmd

 var customExecCommand CustomCommand = exec.Command

+func (sm SAPSystemsMap) GetSIDsString() string {


This duplication looks horrible hehe
I'm sure you will be able to remove this 1st function once you have remove the .Host.GetSIDstring

rtorrero · 2021-07-27T11:40:42Z

@arbulu89 should be fine now! thanks for the comments

arbulu89

Thanks @rtorrero ,
Now it looks great.
I have put some minor comments if you want to have a look

internal/sapsystem/sapsystem.go

web/hosts_test.go

rtorrero · 2021-07-27T12:02:54Z

Sorry again... fixed

rtorrero requested review from fabriziosestito, stefanotorresi, aleksei-burlakov and arbulu89 July 21, 2021 12:58

arbulu89 mentioned this pull request Jul 21, 2021

Add: Output for ha_cluster_discovery and sap_system_discovery #127

Merged

arbulu89 suggested changes Jul 21, 2021

View reviewed changes

arbulu89 reviewed Jul 23, 2021

View reviewed changes

rtorrero requested a review from arbulu89 July 26, 2021 19:02

arbulu89 suggested changes Jul 27, 2021

View reviewed changes

rtorrero force-pushed the issue140 branch from 99add59 to 0e30944 Compare July 27, 2021 11:40

rtorrero requested a review from arbulu89 July 27, 2021 11:40

arbulu89 previously approved these changes Jul 27, 2021

View reviewed changes

internal/sapsystem/sapsystem.go Outdated Show resolved Hide resolved

web/hosts_test.go Outdated Show resolved Hide resolved

web/hosts_test.go Outdated Show resolved Hide resolved

rtorrero added 4 commits July 27, 2021 12:59

tests: expect hosts to possibly have more than one SID

b85b5a8

discovery: allow multiple SID tags per host to be discovered

867caf6

Allow metadata filter query to partially filter one SID

73fa05c

Update templates to show new SID lists

1d3c28a

rtorrero dismissed arbulu89’s stale review via 1d3c28a July 27, 2021 12:02

rtorrero force-pushed the issue140 branch from 0e30944 to 1d3c28a Compare July 27, 2021 12:02

rtorrero requested a review from arbulu89 July 27, 2021 12:03

arbulu89 approved these changes Jul 27, 2021

View reviewed changes

rtorrero merged commit 819088d into trento-project:main Jul 27, 2021

arbulu89 mentioned this pull request Sep 14, 2021

Only 1 SAP system tag is added per node, even in cases with more than 1 SAP installations #140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow multiple SAP system tags per host #160

Allow multiple SAP system tags per host #160

rtorrero commented Jul 21, 2021 •

edited

arbulu89 commented Jul 21, 2021

rtorrero commented Jul 21, 2021

arbulu89 left a comment

arbulu89 Jul 21, 2021

arbulu89 Jul 21, 2021

arbulu89 Jul 21, 2021

arbulu89 Jul 21, 2021

rtorrero Jul 22, 2021

arbulu89 Jul 23, 2021

arbulu89 left a comment

arbulu89 Jul 23, 2021

rtorrero commented Jul 26, 2021 •

edited

arbulu89 left a comment

arbulu89 Jul 27, 2021

arbulu89 Jul 27, 2021

arbulu89 Jul 27, 2021

arbulu89 Jul 27, 2021

arbulu89 Jul 27, 2021

rtorrero commented Jul 27, 2021

arbulu89 left a comment

rtorrero commented Jul 27, 2021

		@@ -77,6 +78,26 @@ type CustomCommand func(name string, arg ...string) *exec.Cmd

		var customExecCommand CustomCommand = exec.Command

		func (sm SAPSystemsMap) GetSIDsString() string {

Allow multiple SAP system tags per host #160

Allow multiple SAP system tags per host #160

Conversation

rtorrero commented Jul 21, 2021 • edited

arbulu89 commented Jul 21, 2021

rtorrero commented Jul 21, 2021

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero commented Jul 26, 2021 • edited

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtorrero commented Jul 27, 2021

arbulu89 left a comment

Choose a reason for hiding this comment

rtorrero commented Jul 27, 2021

rtorrero commented Jul 21, 2021 •

edited

rtorrero commented Jul 26, 2021 •

edited