search

canonical / openstack-exporter-operator

The openstack-exporter-operator is a machine charm for openstack-exporter.
Apache License 2.0
1 stars 7 forks source link

Use the charmed-openstack-exporter snap #85

Closed gabrielcocenza closed 3 months ago

gabrielcocenza commented 3 months ago

Upstream can take some time until releases a new snap and this is blocking the workflow.

This change from using the golang-openstack-exporter (upstream) to use the charmed-openstack-exporter that is packaging the upstream code and release in the snapcraft store.

With this change the charm automatically uses the snap from the snapstore and removes the upstream snap and/or snap as a resource.

The resource can still be used as a way to test customized snap or an alternative solution for airgapped environments

gabrielcocenza commented 3 months ago

@samuelallan72 @chanchiwai-ray after talking with Americas, we decided to remove the possibility of using a custom snap as resource, so the code is reflecting this now. Moreover, I remembered that it's necessary to remove the upstream snap to not conflict with the new one. The snap.remove can be used even if the snap is not present so that is why it's outside the try except block

chanchiwai-ray commented 3 months ago

after talking with Americas, we decided to remove the possibility of using a custom snap as resource, so the code is reflecting this now. Moreover, I remembered that it's necessary to remove the upstream snap to not conflict with the new one. The snap.remove can be used even if the snap is not present so that is why it's outside the try except block

Just want to know the story, why suddenly decide not to use resource. The resource metadata can't be taken down from charmhub, and may not be a good user experience

samuelallan72 commented 3 months ago

I'm not sure removing support for the snap resource is a good idea, so leaving my review in 'request changes' until I understand the rationale.

Following up on this: we discussed internally, and the rationale for removing it was to simplify the charm (user experience and code logic). We discussed airgapped environments, but it was put forward that snap store proxy would be used as the recommended method for handling this.

It does remove the convenience of testing with a custom built snap (eg. if we've working on a new feature or fix in openstack-exporter), but we can workaround that (manually ssh in to the unit and install it).

Something I'm not sure about is the future of the snap resource in charm metadata going forward. I've heard tell that we can't remove this now the charm is published on charmhub. Can we confirm that? If we can cleanly remove the resource from metadata, then I'm fine with going forward with removing it. However, if it's stuck in metadata forever, I'm in favour of retaining support, to avoid confusion in the user experience.

samuelallan72 commented 3 months ago

it looks pretty close now, thanks! I just noted a couple of nits and a query about errors removing the old/upstream snap.

jneo8 commented 3 months ago

Following up on this: we discussed internally, and the rationale for removing it was to simplify the charm (user experience and code logic). We discussed airgapped environments, but it was put forward that snap store proxy would be used as the recommended method for handling this.

It does remove the convenience of testing with a custom built snap (eg. if we've working on a new feature or fix in openstack-exporter), but we can workaround that (manually ssh in to the unit and install it).

Something I'm not sure about is the future of the snap resource in charm metadata going forward. I've heard tell that we can't remove this now the charm is published on charmhub. Can we confirm that? If we can cleanly remove the resource from metadata, then I'm fine with going forward with removing it. However, if it's stuck in metadata forever, I'm in favour of retaining support, to avoid confusion in the user experience.

(non-blocking) I am +1 to keep the snap resource support.

nobuto-m commented 3 months ago

With this change the charm automatically uses the snap from the snapstore and removes the upstream snap and/or snap as a resource.

The resource is removed completely and won't be possible to use it as a resource

The charm declared GA already, and this simply breaks the backward compatibility after that. Can't this part of the change be a separate review?

samuelallan72 commented 3 months ago

this simply breaks the backward compatibility after that.

@nobuto-m could you help us figure out exactly what breaks in the situation of removing the resource? We asked the charm teams and it sounds like the resource will simple silently drop from the charm and the charm will switch from using the custom snap from resource to installing it from the snap store.

nobuto-m commented 3 months ago

this simply breaks the backward compatibility after that.

@nobuto-m could you help us figure out exactly what breaks in the situation of removing the resource? We asked the charm teams and it sounds like the resource will simple silently drop from the charm and the charm will switch from using the custom snap from resource to installing it from the snap store.

One of the motivations to use a snap resource is for an air-gapped environment. I understand that the ideal solution is to use air-gapped mode of snap-store-proxy. However, when upgrading the charm for such an existing environment, does juju refresh just work? I don't think so. That's considered as a major breaking change in that context so separating out the change to drop the resource support would be the best thing for the time being.