allow to enter arbitrary data for the network

[evgeni]

this is useful when the suggestion list is empty

[evgeni]

We could also do something fancy like

Suggested change

	:'data-tags' => 'true',
	:'data-tags' => compute_resource.networks.empty?,

[ShimShtein]

I think it's not needed. Even if you see a partial list, it does not mean that there are no other valid values.

[ekohl]

Can we distinguish between the list is empty because you lack permissions or empty because there are none? Is the latter even a realistic case?

[evgeni]

We could make

foreman_kubevirt/app/models/foreman_kubevirt/kubevirt.rb

Lines 80 to 86 in 63e77c2

	def networks
	client.networkattachmentdefs.all
	rescue StandardError => e
	logger.warn("Failed to retrieve network attachments definition from KubeVirt,
	make sure KubeVirt has CNI provider and NetworkAttachmentDefinition CRD deployed: #{e.message}")
	[]
	end

return something else than an empty list, which would make it distinguishable, but I am with Shim here, I'd keep it simple and always allow it.

[ekohl]

The logger.warn message is not visible for the user because it's only in the logs.

My main worry is that is may not be clear to the user why it's a text field and not a drop down. I'm tempted by the idea of returning nil so the frontend can distinguish but also worried about introducing new bugs with that.

If we go this route, I'd also suggest StandardError is too generic and looking at a more specific exception for a missing permission would be better.

Also toying with the idea of flashing an info/warn box in the except block, but that's also not great for the user to act on.

Perhaps short term the best we can do is to document the requirement. I'd also really like it if the Test connection button could show a warning that the permission is missing. Shall I write up a story for that so we don't lose track of it?

[ShimShtein]

@ekohl since it's totally acceptable not to have that list populated, I think I would avoid warning the user. Now that I think about it, I am not sure log.warn is appropriate here too. It's not something that the user/foreman admin should do something about.

[ekohl]

I'd say we should catch the permission error and that could be a logger.warn but StandardError should be IMHO be logger.exception.

[evgeni]

The permission error is a Kubeclient::HttpError, so better than StandardError, but still not really specific :/

[evgeni]

Mhh, https://github.com/ManageIQ/kubeclient/blob/v4.y/lib/kubeclient/http_error.rb says

# TODO: remove this on next major version bump
# Deprected http exception

but https://github.com/ManageIQ/kubeclient/blob/master/lib/kubeclient/http_error.rb is still there for 5.x

but then again, this particular comment was introduced during 2.x→3.x and so far remains there
ManageIQ/kubeclient@87b8ff1

[evgeni]

and of course fog hides that and we get a Fog::Kubevirt::Errors::ClientError, jeez

[evgeni]

This works fine for creation, but when you try to edit (a host or a compute profile, which uses the same form), the value gets reset, as the custom value we injected is not part of the compute_resource.network list above

[evgeni]

okay, adding it explicitly works, but ewww

[evgeni]

it probably should be .compact.uniq, but I bet the openstruct thing won't uniq with a real network anyway 😭

[ShimShtein]

You can use the block parameter in uniq to compare different classes of objects: https://apidock.com/ruby/Array/uniq