| *** pmannidi is now known as pmannidi|afk|1hr | 02:26 | |
| *** pmannidi|afk|1hr is now known as pmannidi | 05:01 | |
| ltomasbo | digitalsimboja, having issues to connect? | 09:12 |
|---|---|---|
| simboja | @maysams, @ltomsabo: I was having challenges with my connection | 09:16 |
| simboja | My apologies | 09:16 |
| maysams | simboja: okay, I just emailed you, we can sync over email/here | 09:16 |
| simboja | okay | 09:17 |
| simboja | I am in on Google meet now | 09:19 |
| simboja | But it keeps kicking me out after few seconds | 09:19 |
| simboja | Could we reschedule please? | 09:20 |
| ltomasbo | simboja, sure, lets sync here and have a short meeting later this week (tomorrow/wednesday) if needed | 09:20 |
| simboja | Alright perfect! | 09:21 |
| simboja | So I got your reviews | 09:21 |
| ltomasbo | simboja, maysams and I discussed that maybe it is good if you split your patch in 2, one for the previously existing loadbalanacer reconciler | 09:21 |
| ltomasbo | and another one for the listeners/pools... as a follow up | 09:21 |
| ltomasbo | that way we can enhance and merge first the loadbalancer one, and then enhance it with the listeners/pools reconciler later on | 09:21 |
| ltomasbo | perhaps this week you can focus on, after spliting your current patch set in 2, adding unit test to it | 09:22 |
| simboja | Yeah, that would make sense. | 09:22 |
| maysams | yup | 09:22 |
| ltomasbo | (to the loadbalancer one) | 09:22 |
| ltomasbo | would be nice to have that patch in mergable status this week | 09:22 |
| ltomasbo | so that you can focus on tempest testing and/or listeners/pools in the following one | 09:22 |
| simboja | sure | 09:23 |
| ltomasbo | (or even in this one) | 09:23 |
| ltomasbo | what questions did you have for the reviews? | 09:23 |
| simboja | sure! regarding the utility function that gets the resources from k8s, | 09:24 |
| simboja | Per my understanding, I need to retain the functions but have them call the ge_resource function individually? | 09:24 |
| simboja | pssing the resource name and resource path correct? | 09:25 |
| ltomasbo | yep, main idea is to avoid having code duplication | 09:25 |
| ltomasbo | you can have both functions you use to have... and instead of both having mostly the same code, just having some auxiliar function that they call | 09:25 |
| simboja | I got it! I will rework it asap | 09:25 |
| ltomasbo | like _get_crd_resource | 09:25 |
| ltomasbo | great! | 09:25 |
| simboja | Then regarding https://review.opendev.org/c/openstack/kuryr-kubernetes/+/798904/12/kuryr_kubernetes/controller/handlers/loadbalancer.py#188 | 09:26 |
| ltomasbo | also, after you added the listeners, we realized that you were iteraring a list (list((loadbalancer_crd.get('status', {}).get( | 09:26 |
| ltomasbo | 'loadbalancer', {}).get('id', {}),) | 09:26 |
| ltomasbo | yep, the same... perhaps you can use a dict instead, were you have the id pointing to the selfLink | 09:27 |
| simboja | I would need to check if the listeners are mapped to the approriate loadbalancer? | 09:27 |
| ltomasbo | {lb_id: selflink} | 09:27 |
| ltomasbo | then, instead of later you having to use lb_id[1] and lb_id[0], which is harded to know what they are refering to | 09:27 |
| ltomasbo | you can simply use lb_id['id' | 09:27 |
| maysams | I don't think there is a need to check if it's mapped to the appropriate lbs | 09:28 |
| simboja | I will consider that, the current implementation works though its a lists within list | 09:28 |
| maysams | it's just to use dicts instead as ltomasbo mentioned | 09:28 |
| simboja | Got it! | 09:28 |
| simboja | I will rework this too | 09:28 |
| maysams | it's much clear which specific resource is being retrieved when using dicts | 09:28 |
| simboja | Then split the patchset | 09:29 |
| ltomasbo | ahh, for the other comment... that is related to the listeners... that can be moved to a follow-up patch... and we can handle then | 09:29 |
| simboja | sure! @ maysams | 09:29 |
| simboja | sure! @ltomasbo | 09:29 |
| ltomasbo | great! | 09:29 |
| maysams | simboja: anything extra regarding the recent reviews? | 09:31 |
| simboja | Thanks! | 09:31 |
| simboja | Let me focus on getting the loadbalancer loop mergable so we can batch that then focus on the listeners then the rest? right? | 09:31 |
| simboja | No @maysams | 09:31 |
| simboja | at least for now | 09:31 |
| maysams | okay | 09:32 |
| simboja | I will take a look at what you shared about connecting the patch to the blueprint | 09:32 |
| simboja | then come back to you if i have questions | 09:32 |
| maysams | simboja: yes, split the patch and get the loadbalancer one ready to merge, which means to also include unit tests | 09:33 |
| simboja | sure! | 09:34 |
| opendevreview | Merged openstack/kuryr-kubernetes master: gracefully exit daemonserver before registry exit https://review.opendev.org/c/openstack/kuryr-kubernetes/+/698618 | 16:29 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!