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

Implement the SAP systems payload decoding and usage #602

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

arbulu89
Copy link
Contributor

Decode the SAP systems discovery payload with ECTO.

Caveats:

  • I have done some code enrichments in the decoding side, as it helps to reduce a bit the code boilerplate in the parsing. Specially, the current_instance entry, which is useful to know which sap instance is being executed in the current discovered host
  • Due to the Jason encoding errors. I couldn't use the Trento.Type schema type in the embeds where some keys have /, like
    the Profile embed. I couldn't really create a fix. Feel free to propose alternatives
  • I couldn't use the ProperCase option to snake case all the keys, as it creates some really strange entries pending to review, as we don't use most of those strange fields

@arbulu89 arbulu89 force-pushed the parse-sap-system-payload branch from 0a55f12 to fa845fe Compare May 27, 2022 14:09
@arbulu89 arbulu89 marked this pull request as ready for review May 30, 2022 06:24
@arbulu89 arbulu89 added the enhancement New feature or request label May 30, 2022
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.

This was funky, wasn't it? 😄
Jokes apart, looks really good, thanks!

In relation to the keys having / I am wondering if it would help using sort of a map between keys with slashes and keys without slashes and let Jason handle only no-slash keys.

I also left a comment on :Capitalized fields, which might be related to the pain points you're raising.

use Trento.Type

deftype do
field :Id, :string
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, fields should be starting with a lowercase.

Is this related to your comment
I couldn't use the ProperCase option to snake case all the keys, as it creates some really strange entries ?

If so I couldn't fully understand what is happening 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fields are exactly what we receive in the discovery json payload. In this case we receive the Id, as it is, starting upper case.
In other piece of code, @dottorblaster used the ProperCase.snake_case function to convert the coming payload to snake-cased payload, so he could have snake case fields. In essence, the payload schemas he created doesn't 100% match with the coming payload, as he is pre-snake-casing (nice word XD).

The reason why I didn't use the ProperCase.snake_case function is that it creates really strange keys, because our coming payload has strange keys indeed. Many of them because we simply send but SAP commands give us...

Copy link
Member

@fabriziosestito fabriziosestito May 30, 2022

Choose a reason for hiding this comment

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

@arbulu89
could you please make an example of what are these strange keys? maybe I would be able to help with this or even rewrite our version of ProperCase that fits for our use-case (including the "/" keys)

Copy link
Contributor Author

@arbulu89 arbulu89 May 30, 2022

Choose a reason for hiding this comment

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

@fabriziosestito
Here some few:

  • HdbnsutilSRstate to hdbnsutil_s_rstate
  • icm/HTTP/ASJava/disable_url_session_tracking to icm/_htt_p/_as_java/disable_url_session_tracking
  • is/HTTP/show_detailed_errors to is/_htt_p/show_detailed_errors
  • siteTier/NBG to site_tier/_nbg

From this list, we really only use the 1st one. I didn't want to start having this strange fields in the code.
I thought of changing the agent side too, but I didn't want to start on this until having the payload versioning system on place. In in may cases, these strings are coming directly from SAP utilities, so we cannot do changes that easy.

If you don't mind having these fields, we can snake case anyway

Copy link
Member

Choose a reason for hiding this comment

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

@arbulu89
I see thanks. I started playing around but I ended up with the same problem as the propercase library. It uses Macro.underscore under the hood which causes the "weird" casing. We will want to change the agent side in the future I agree. Since there is no easy way to do this, and what you proposed is limited to the payload decoded (and not to the commands itself) I would think this is good enough (even if I cringe everytime I see a camlecase atom field 😬 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can have poll hehe;

Who prefers the:

  • ugly CamelCase entries
  • ugly snake_case entries

Either way, there will be strange fields.

@arbulu89
Copy link
Contributor Author

@nelsonkopliku On this:

In relation to the keys having / I am wondering if it would help using sort of a map between keys with slashes and keys without slashes and let Jason handle only no-slash keys.

That's a tricky thing. The slash based keys are coming from SAP commands outputs, and we simply forward them from the agents. And some of those keys are used by our app, and some are not.

I thought of simply using :map as type, but this would remove the option to use the Ecto in-built validation methods, which are basically why we are decoding with Ecto I guess. Because I would need to define as map, and later on implement my own validation to see if those maps have the required fields, and if they don't, raise a validation failure.
We could do this with Ecto.validate_change I guess.
The end result will be the same in any case, but we would have less custom modules defined

@arbulu89 arbulu89 force-pushed the parse-sap-system-payload branch from 429d4a5 to d3054ce Compare May 30, 2022 12:52
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.

Let's go with this I'd say.
We can iterate over.

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.

LGTM

@arbulu89 arbulu89 merged commit 9a19c39 into main Jun 1, 2022
@arbulu89 arbulu89 deleted the parse-sap-system-payload branch June 1, 2022 11:53
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.

4 participants