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

Remove ambiguity between ConfigObject::GetZone() and Endpoint::GetZone() #8740

Open
julianbrost opened this issue Apr 26, 2021 · 3 comments
Open
Assignees
Labels
core/quality Improve code, libraries, algorithms, inline docs TBD To be defined - We aren't certain about this yet

Comments

@julianbrost
Copy link
Contributor

Currently, there exist both ConfigObject::GetZone() and Endpoint::GetZone(). As Endpoint inherits from ConfigObject, it is shadowed but both are non-virtual and behave differently:

  • ConfigObject::GetZone() returns the zone that the object is configured within and is relevant for the config sync.
  • Endpoint::GetZone() returns the zone that endpoint is a member of (as configured in the endoints attribute of a zone) which is relevant for cluster routing.

This leads to confusing code where GetZone() of the same object behaves differently depending on its static type:

if (type == Endpoint::TypeInstance) {
ObjectsZone = static_cast<Endpoint*>(object.get())->GetZone();
} else {
ObjectsZone = static_pointer_cast<Zone>(object->GetZone());
}

Also, when discussing it internally, it was quite hard to refer to both of these zone relationships so we should come up with better and more distinctive naming.

@julianbrost julianbrost added the core/quality Improve code, libraries, algorithms, inline docs label Apr 26, 2021
@Al2Klimov Al2Klimov added the good first issue Good for newcomers label Oct 12, 2021
@Ishvina
Copy link

Ishvina commented Jan 20, 2022

Hi! I am in an intro to swe class that is requiring me to work on my first open-source project. I was wondering if this issue is still open and if so could is this beginner-friendly?

@julianbrost
Copy link
Contributor Author

Hi, thanks for your interest in contributing. This issue is still open and the resulting fix won't need any major code changes, however, the issue requires a good understanding of Icinga 2 and especially its cluster functionality to come up with a meaningful new suggestion for naming things.

@Al2Klimov Al2Klimov removed the good first issue Good for newcomers label Mar 8, 2022
@Al2Klimov Al2Klimov added the TBD To be defined - We aren't certain about this yet label Mar 29, 2022
@Al2Klimov
Copy link
Member

What about GetEndpointZone() for referring to the zone of specifically the endpoint?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/quality Improve code, libraries, algorithms, inline docs TBD To be defined - We aren't certain about this yet
Projects
None yet
Development

No branches or pull requests

3 participants