search

dwellir-public / polkadot-operator

The Polkadot Juju operator/charm
Apache License 2.0
1 stars 1 forks source link

Parachain-spec-url #36

Closed jonathanudd closed 9 months ago

jonathanudd commented 9 months ago

I began implementing a resource for this but I still don't see how a resource is superior to a config in this case. To me it feels like unnecessary work to download spec files and upload them as resources to charmhub instead of just setting the url in the config. If we want to keep one spec file per chain we would also need one resource file entry in the metadata.yml file if I'm not missing something.

The main benefit I see with resources is that when you need to make changes in a spec file it would be more convenient to download it, make the change and attach it as a resource to the application in the model. I needed this for Subsocial so with the code in this PR I downloaded the file modified it and uploaded it on nextcloud to use as url to the config.

Do you guys want to have a workshop or meeting to discuss how we want to handle spec files in the Polkadot charm? I see three options

  1. Keep adding hardcoded urls in the service-args file.
  2. Add resource support for spec files
  3. Add a config for spec files

I will deploy a Subsocial node with a locally built charm based on this code for now to get a node running even if we don't merge this in the end.

Maharacha commented 9 months ago

I agree with you about the resource feature. I have never seen a good use-case for it. Would be interesting to see that. Maybe @erik78se knows what the benefits could be? I think it's good to move the spec-file thing to config. The requirement for --chain should also be removed then, if spec-file is set in config. It will be nice to get rid of the made-up names for the --chain arg.

Maharacha commented 9 months ago

I agree with you about the resource feature. I have never seen a good use-case for it. Would be interesting to see that. Maybe @erik78se knows what the benefits could be? I think it's good to move the spec-file thing to config. The requirement for --chain should also be removed then, if spec-file is set in config. It will be nice to get rid of the made-up names for the --chain arg.

Use-case for resources could of course be when it's not possible to get it or put it online somewhere. Offline deployments... But yeah, not really relevant in this case.

jakobilobi commented 9 months ago

Do you guys want to have a workshop or meeting to discuss how we want to handle spec files in the Polkadot charm?

We could, or we could also just discuss it here.

I also agree with the resource commentary above, I think I wrote it in some other PR/thread also. Not seeing the benefits. I'm also for moving specfile URL:s to a config over having them hard coded. Though we should make sure we cover the case where a spec file is needed for both relay and para on the same node, because some have that requirement if I'm not mistaken?

Offline deployments...

What's beyond web3? The offline blockchain :D

Maharacha commented 9 months ago

I was also thinking that it should support chain spec for relay. It's not common but it exist. Supporting that and remove the need for --chain when using it, and let's merge!

jonathanudd commented 9 months ago

Any other opinions about the code @Maharacha @jakobilobi ? If so I could fix any issues when adding support for relay chain spec files. I will probably do it tomorrow.

jonathanudd commented 9 months ago

I made some additions. Not sure if it's optimal but it works. I will do some additional tests as well.

remove the need for --chain when using it

The chain argument is still needed in the cases of downloading binaries from docker. Should we remove the requirement anyway?

Leave it as it is for now. We can get rid of it when we get rid of the docker extract.

jonathanudd commented 9 months ago

I have tested this with only parachain, only relaychain, only live network, parachain and local relay. Setting the configs during deploy, after deploy and un-setting the config for the scenarios above. Also tested for chains which have hard coded overrides where the config overrides the hardcoded value.

jonathanudd commented 9 months ago

I noticed one thing that I wont address in this PR. The ServiceArgs class is used throughout the charm in almost all hooks and actions and since we download the chain spec files when this class is used the spec files will be downloaded every time it is used. This means that the spec files will be downloaded and replaced every time the status hook runs.

This is of course not optimal but seem to not cause any issues either most of the time.