Runs have a UUID, but Radicle CI broker doesn't require that — radicle-job

Radicle Job Collaborative Object

Runs have a UUID, but Radicle CI broker doesn't require that

liw opened 10 months ago

In https://app.radicle.xyz/nodes/seed.radicle.garden/rad:z2UcCU1LgMshWvXj6hXSDDrwB8q8M/tree/src/lib.rs#L139, when we insert a new run to the list of runs for a job, radicle-job requires it to be a Uudi:

 pub fn insert(&mut self, uuid: Uuid, run: Run) -> Option<Run> {

On its own, this is fine. The Radicle CI broker assigns a run ID that is a UUID. However, so far, that has been an internal implementation decision for the CI broker. Would it be OK for radicle-job to accept a string instead of a UUID here? If not, I can make the internal implementation decision be an API promise to make sure to stay compatible with radicle-job. It feels important that the CI broker and the COB use the same identifier for a CI run. If they're not, it will be difficult to programmatically identify which COB refers to which CI run.

I note that being a UUID doesn't guarantee uniquess, when the same UUID can be reused, so changing it to String doesn't seem like a significant loss in uniqueness to me.

z6MkireR...3voM commented 10 months ago

Ya, so I thought that Uuid would be a common identifier for a Run, but I'm happy to change this. Perhaps it can be made generic so that implementors of different Jobs can decide what identifier they are using. Maybe this boils down to String, but I'd prefer let implementors decide what type they use for their own type safety benefits.

The only constraints for the keys is to be Hash + Eq for the underlying IndexMap.

liw commented 10 months ago

Tecnically, radicle-ci-broker defines RunId (in its src/msg.rs) as:

pub struct RunId {
    id: String,
}

When radicle-ci-broker invents a new ID, it creates a UUIDv4 and turns that into a string: Uuid::new_v4().to_string()

However, run IDs also come from the adapter, which may get them from anywhere, and there is no guarantee they are unique. Hence String in RunId.

I could split "broker run ID" and "adapter run ID" to be separate types.

So I see three options:

status quo: radicle-job keeps Uuid, radicle-ci-broker only gives -job strings that are from Uuid, so everything works
I split RunId into RunId (for adapters; type name can't change without breakage) and BrokerRunId which contains a Uuid
radicle-job changes to contain a String

Do you have preferences?