ContainerServicePortConfigs using [:container_service, :name]

[Ladas]

ContainerServicePortConfigs using [:container_service, :name]
as manager_ref

[Ladas]

@miq-bot assign @agrare
@miq-bot add_label bug

[Ladas]

@cben @simon3z ok so continuing ManageIQ/manageiq-providers-kubernetes#89 (comment)

But what is :name exactly, I see that we test it's nil in https://github.com/Ladas/manageiq-providers-kubernetes/blob/c89165a29404a3903030ed86ec2cb34a68e53329/spec/models/manageiq/providers/kubernetes/container_manager/refresher_spec.rb#L260 , so can we really use it?

(Also, using :name would make ems_ref unused, so it can be removed.)

[miq-bot]

Checked commit Ladas@83f0e9f with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[cben]

https://v1-5.docs.kubernetes.io/docs/api-reference/v1/definitions/#_v1_serviceport

Name	Description	Required	Schema	Default
name	The name of this port within the service. This must be a DNS_LABEL. All ports within a ServiceSpec must have unique names. This maps to the Namefield in EndpointPort objects. Optional if only one ServicePort is defined on this service. [emphasis mine]	false	string

AFAICT it's always been like this. 1.8 api doc says the same. didn't find 1.4 api docs but user guide says the same.
1.0 source code and 1.8 source code say the same.

=> LGTM, it's always unique within a service, just sometimes nil (and possibly sometimes ""? I saw "" in old kubectl sources while searching this).

Do we need any extra tests for nil being treated as distinct from other values?

[Ladas]

@miq-bot remove_label wip

[Ladas]

@cben awesome, hm might be add extra specs. You are thinking about manual VCR modification?

[cben]

No, I want to keep the newer vcr re-recordable, free of manual edits, which is why I mentioned this reluctantly ;-), IMHO not worth the trouble to re-record. I'd like to spend a few days making it completely automated with ansible or something, when it's a quiter time... Perhaps add somewhere unit tests for save_inventory / ManagerRefresh, if you have any concrete suspicion in what way nil might fail. 2017-11-14 14:36 GMT+02:00 Ladislav Smola <[email protected]>:

…

@cben <https://github.com/cben> awesome, hm might be add extra specs. You are thinking about manual VCR modification? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16454 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQtGHnBEkPspg8lj-rTVeWG8-ggU5E0ks5s2YlKgaJpZM4QbsHo> .

[cben]

Sorry, neglected all this. LGTM 👍. @agrare can we merge this?

Found example in current VCRs => added test in ManageIQ/manageiq-providers-openshift#86, fails classic refresh now, passes with this PR.

I don't remember if this is user-visible, will check soon to see if worth backporting.

[agrare]

👍 LGTM

[cben]

UPDATE: Yes, it's user-visible. Pre-fix (classic refresh):

fixed:

fine/yes? I suppose I need to open a BZ...