search

buildstream-migration / bst-staging

GNU Lesser General Public License v2.1
0 stars 0 forks source link

ArtifactCacheSpec should contain CASRemoteSpec instead of inheriting it #785

Open Cynical-Optimist opened 4 years ago

Cynical-Optimist commented 4 years ago

See original issue on GitLab In GitLab by [Gitlab user @jmacarthur] on Nov 26, 2018, 11:43

Background

Recording some technical debt which went into !946: As [Gitlab user @danielsilverstone]-ct points out, ArtifactCacheSpec should properly contain a CASRemoteSpec, rather than being one, since an ArtifactCache contains CASRemote objects.

ArtifactCacheSpec objects are created both by their direct __init__ method and _new_from_config_node. If we want to implement both in a containing class, we need a second constructor for ArtifactCacheSpec objects, which means messing around with __new__, or breaking up CASRemoteSpec's _new_from_config_node.

Task description

Acceptance Criteria

ArtifactCacheSpec contains a CASRemoteSpec instead of inheriting it and functionality is unchanged.

Cynical-Optimist commented 4 years ago

In GitLab by [Gitlab user @jmacarthur] on Nov 26, 2018, 11:43

mentioned in merge request !946

Cynical-Optimist commented 4 years ago

In GitLab by [Gitlab user @raoul].hidalgocharman on Jan 9, 2019, 10:58

mentioned in merge request !1013

Cynical-Optimist commented 4 years ago

In GitLab by [Gitlab user @tristanvb] on Jan 9, 2019, 18:01

I think this spec parsing needs further thought.

Currently I think it's tangled up that yaml parsing is done in the artifact storage delegates.

I think that the CAS should expose a pure python API for configuration, and the ArtifactCache should proxy it. User facing configuration in ~/.config/buildstream.conf should be loaded by Context in the regular way, and either applied directly to the underlying caches, or stored on the Context to be applied later in the initialization.

See also https://gitlab.com/BuildStream/buildstream/merge_requests/1013#note_129899086