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

Discover angi architecture #2761

Merged
merged 9 commits into from
Jul 10, 2024
Merged

Discover angi architecture #2761

merged 9 commits into from
Jul 10, 2024

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Jul 9, 2024

Description

Discovery of angi architecture HANA clusters.
A new field, architecture_type has been added to the hana cluster details, being "classic" and "angi" the possible values.

The architecture_type field is now available in the clusters api.

@abravosuse What do you think of the field name architecture_type?

How was this tested?

UT added

@arbulu89 arbulu89 added enhancement New feature or request env Create an ephimeral environment for the pr branch regression Add this label to force the PR to run upcasting regression tests labels Jul 9, 2024
@arbulu89 arbulu89 force-pushed the discover-angi-architecture branch from 08d67c8 to 2e7e789 Compare July 9, 2024 10:16
@arbulu89 arbulu89 requested review from CDimonaco and EMaksy July 9, 2024 11:40
@arbulu89 arbulu89 marked this pull request as ready for review July 9, 2024 12:45
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Good job 🚀

I left some comments, up to you, they are code style comments


# angi architecture follows a really similar configuration to the scale out one.
# That's why some functions are shared with classic scale out setup
defp parse_cluster_details(
Copy link
Member

Choose a reason for hiding this comment

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

Totally ok with the comment, the other option was to override any of the shared function, with the prefix angi and proxy them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the end, i was having already enough code duplication hehe
In any case, whoever looked for the proxy, would have the same questions, why angi is using scale out code XD

@arbulu89
Copy link
Contributor Author

arbulu89 commented Jul 9, 2024

Hey @CDimonaco
I have updated the payload code.
Now the hana_architecture_type and cluster_type are set together.
The code looks cleaner and we don't use that many functions.
I have moved the enrich_cluster_sid function below, that's why you might see some more changes

@arbulu89 arbulu89 closed this Jul 9, 2024
@arbulu89 arbulu89 reopened this Jul 9, 2024
@arbulu89
Copy link
Contributor Author

arbulu89 commented Jul 9, 2024

Damn, clicked wrong button XD

@arbulu89 arbulu89 force-pushed the discover-angi-architecture branch from 6d08c98 to 78d6bcb Compare July 9, 2024 15:11
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

Beautiful!

Thanks for addressing the feedbacks, I left a very little nitpick, as you prefer

LGTM! 🚀

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey man, code wise lgtm!

@arbulu89 arbulu89 merged commit 5d2edb7 into main Jul 10, 2024
26 checks passed
@arbulu89 arbulu89 deleted the discover-angi-architecture branch July 10, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request env Create an ephimeral environment for the pr branch regression Add this label to force the PR to run upcasting regression tests
Development

Successfully merging this pull request may close these issues.

3 participants