Initial support for removing dynamically imported artifacts. #3716
Conversation
mitchell-as
force-pushed
the
mitchell/cp-956
branch
from
9ee5e09 to
84ffb1e
Compare
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
Nice work! Just some minor churn.
pkg/runtime/depot.go
Outdated
| func (d *depot) Track(id strfmt.UUID, deploy *deployment) error { | ||
| // The artifact parameter is only necessary for tracking dynamically imported artifacts after being | ||
| // added by an ecosystem. | ||
| func (d *depot) Track(id strfmt.UUID, deploy *deployment, artifact *buildplan.Artifact) error { |
There was a problem hiding this comment.
| func (d *depot) Track(id strfmt.UUID, deploy *deployment, artifact *buildplan.Artifact) error { | |
| func (d *depot) Track(artifact *buildplan.Artifact, deploy *deployment) error { |
Could you try to make this the behaviour? The reason this was using id was to keep it as dumb as possible. Obviously for artifacts that ship has now sailed, I'd prefer we're consistent then in so far as that is possible.
This also means updating the Deploy functions. The real question mark here is going to be the Save() function, which far as I can tell is "abusing" the Track() function in order to clean up the cache. I don't recall specifically what the exact behaviour is there nor can I quickly grok it, but I would suggest spending some time there to try and see if we can remove its reliance on Track(). If this turns into a deep rabbit hole please push back.
pkg/runtime/setup.go
Outdated
| } | ||
|
|
||
| // If this is a dynamically imported artifact, tell the ecosystem to remove/undeploy it. | ||
| if artifact, exists := s.depot.config.Cache[id]; exists && artifact.Namespace != "" { |
There was a problem hiding this comment.
Could you update the depot.Exists() function to return the artifact? I'd prefer we don't access private properties on the depot to keep the responsibilities clear.
pkg/runtime/setup.go
Outdated
| if ecosys := filterEcosystemMatchingNamespace(s.ecosystems, artifact.Namespace); ecosys != nil { | ||
| installedFiles := []string{} | ||
| // Find record of our deployment | ||
| if deployments, ok := s.depot.config.Deployments[id]; ok { |
There was a problem hiding this comment.
Please make a public method on the depot to access this. Possibly you could even combine this with the artifact,extists logic above seeing as the deployment has the namespace in it. Something like depot.GetDeployments(id) []Deployment. Then check the namespace off os those.
pkg/runtime/setup.go
Outdated
| installedFiles := []string{} | ||
| // Find record of our deployment | ||
| if deployments, ok := s.depot.config.Deployments[id]; ok { | ||
| deployments = sliceutils.Filter(deployments, func(d deployment) bool { return d.Path == s.path }) |
There was a problem hiding this comment.
Might need fileutils.ResolveUniquePath() here. Note I thought we had something like fileutils.IsEqualPath() but apparently not, could be worth adding.
mitchell-as
force-pushed
the
mitchell/cp-956
branch
from
7216582 to
3998736
Compare
There was a problem hiding this comment.
Nice work! Definitely feel like the depot is in a poor state right now due to the mix of responsibilities with DI, and the Deploy methods / Track methods stick out like a sore thumb. But that's a bigger issue, I like that this PR is going as far as reasonable to keep those painpoints minimal.
|
I completely agree that the responsibilities need to be reworked. It definitely feels dirty. |
I could not test Java and Rust because the language cores are not building. However, Rust is pretty much identical to .NET with how we are vendoring artifacts and Java is also similar, but with individual jars instead of directories. I am confident they work as expected.