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

Add SBD_OPTS to cluster discovery events #996

Merged
merged 1 commit into from Nov 29, 2022

Conversation

nelsonkopliku
Copy link
Member

Description

This change in the agent trento-project/agent#145 exposes to discoveries also empty SBD configuration entries like SBD_OPTS= (becoming in the json "SBD_OPTS": "")

With this PR we test that the change is supported by trento web as well.

How was this tested?

Changed fixtures and let e2e tests run.

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.

Photofinish scenarios are not supposed to fiddle with, mainly because the majority of the fixtures we have is extracted from real landscapes and should reflect what the customers are running.

trento-project/agent@c51ca4e#r1027690491

in case we could remove the extra entries so the output is the same to what we had with the regex

I think this is spot on, maybe we could pursue that path?

@arbulu89
Copy link
Contributor

Photofinish scenarios are not supposed to fiddle with, mainly because the majority of the fixtures we have is extracted from real landscapes and should reflect what the customers are running.

trento-project/agent@c51ca4e#r1027690491

in case we could remove the extra entries so the output is the same to what we had with the regex

I think this is spot on, maybe we could pursue that path?

@dottorblaster In the other hand, I think this is a pretty logical movement.
I think that the change @nelsonkopliku did caught an "weakness" our discovery process had, not sending empty values.
I think it is valuable to have them.
Here the thing is that we don't even use the sbd config data in the web side...
Most probably we did in the old golang based backend, but we don't do it anymore.
That's why this change is harmless.
I don't know if the users would have those values changes (99% not) but modifying this values is something that we should consider.
As the agent evolves, the payloads must evolve together

@nelsonkopliku
Copy link
Member Author

@arbulu89 @dottorblaster with this trento-project/agent#145 being merged it looks safe enough updating the fixtures accordingly.

In addition, the updated fixtures is what would be extracted from real landscapes and would reflect what the customers are running (ofc I am referring to the latest version of the agent)

@arbulu89
Copy link
Contributor

@arbulu89 @dottorblaster with this trento-project/agent#145 being merged it looks safe enough updating the fixtures accordingly.

In addition, the updated fixtures is what would be extracted from real landscapes and would reflect what the customers are running (ofc I am referring to the latest version of the agent)

I'm fine with it. Mostly, because it is pretty irrelevant as we don't use this data anywhere (so it is not harmful for sure). But it is true that this is what the agent would be sending so I'm in favor of the change.

@nelsonkopliku nelsonkopliku force-pushed the add_extra_sbd_discovered_config branch 4 times, most recently from e34a2f0 to 3bdcb16 Compare November 29, 2022 11:27
Copy link
Member

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

Missing verified commit

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.

I don't like the idea of fiddling with fixtures but as long as we verify that this is exactly the payload that we can expect... I think we don't have really a choice 😄

@nelsonkopliku nelsonkopliku merged commit f2bbb28 into main Nov 29, 2022
@nelsonkopliku nelsonkopliku deleted the add_extra_sbd_discovered_config branch November 29, 2022 15:46
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.

None yet

4 participants