-
Notifications
You must be signed in to change notification settings - Fork 11
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
Sap profile gatherer #267
Sap profile gatherer #267
Conversation
Edit:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments
@@ -80,7 +83,7 @@ func NewSAPSystemsList( | |||
var systems = SAPSystemsList{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's old code but we want to refactort this to the default/new constructor convention we have in the other gatherers? To inject dependencies in tests and the default one with the real scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get your point (even though this is not a gatherer code).
Refactoring a bit the code would allow us to test better (in fact, this particular function is not even tested...).
Do you mind if I open a tech-debt ticket in our backlog?
The refactor might not be so trivial, and I don't want to include it in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @arbulu89, thanks! just left you a minor question in the code 👍
@@ -227,12 +231,28 @@ func findInstances(fs afero.Fs, sapPath string) ([][]string, error) { | |||
return instances, nil | |||
} | |||
|
|||
func getProfilePath(sysPath string) string { | |||
return path.Join(sysPath, "SYS", "profile", sapDefaultProfile) | |||
// FindProfiles returns the latest profile file names in the /sapmnt/${SID}/folder folder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you refer to the "latest" profile names and a bit below, on line :246 you output "New SAP profile found"...
Are these really new or will they be always found everytime this code is run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, latest in this context means the "latest valid SAP profile".
SAP has its own backing up system, where old backup files are stored in the same folder, with .x
terminology (where x is a digit). It is a bit different for the DEFAULT.PFL
but the concept is the same.
The logging simply prints all found profiles.
This code is deterministic, it will always return the same, as long as the Filesystem is the same of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
Edit: I have decide finally to go with the map way. See comments below
Edit2:: In the future, we could upgrade this gatherer to return the installed SAP instances per SID in the host, which will be really needed in many checks. We could reuse this gatherer instead of creating a new one
SAP profiles gatherer. It walks the
/sapmnt/<SID>/profiles
folder to look for profiles.It only gets the latest profile for each instance (so files ending with numbers or old DEFAULT files are excluded).
It returns a flat structure adding the sid as field.
I have some doubts if it should go in this way, or in "sid as map" kind of tree. Unfortunately
rhai
doesn't have thegroupBy
feature. Wdyt?See example: