diff internals: Introducing a DiffResource(s) struct

[criztovyl]

There are some methods in changeset_with_versioned_rs.go that could qualify for a dedicated type/struct:

I think I would prefer it if this function [newGroupedVersionedResources] and the next one [existingResourcesMap] are either scoped to a struct, maybe we can move stuff that are used by other parts of the code to a single place at a later point. Not a blocker but a nice to have

Originally posted by @100mik in #571 (comment)

The changes introduced in #571 actually introduced a conflict with "far-reaching" consequences to my #517 PR.
(71 and 17, what a coincidence!)

I thought about the following there, what do you think about it?
I would try to implement that in a dedicated PR.

Note that My PR also introduces an abstraction for VersionedResource, but i never was quite happy with that name, maybe that could be lifted up into a DiffResource, extracting it from my PR into the new/dedicated PR?

#571 introduced a conflict on func (d ChangeSetWithVersionedRs) groupResources([]ctlres.Resource): it moved it to func newGroupedVersionedResources(...) and uses it via diff.existingResourcesMap. But I changed the method to use (and return) []VersionedResource, instead of instantiating VersionedResource ad-hoc.

To resolve that conflict I think I need to implement the suggestion from #571 (comment), extracting existingVersionedResources and newVersionedResources to a new struct.

[...]

// cmd/app/deploy.go
// func (o *DeployOptions) calculateAndPresentChanges(...)
//...
diffResources := ctldiff.NewDiffResourceFactory(existingResource, newResources,
    conf.TemplateRules(), conf.StripNameHashSuffixConfigs()).NewDiffResources()

err := ctldiff.NewRenewableResource(diffResources).Prepare()
// ...

changes, err := ctldiff.NewChangeSetWithVersionedRs(diffResources,
    opts ChangeSetOps, changeFactory ChangeFactory).Calculate()
// ...

with:

package diff

type DiffResources struct { // or any other name, only was the first one that came to my mind ^^
    ExisitingResources, NewResources versionedResources
    ExistingResourcesGrouped map[string][]VersionedResource
    // NB: While RenewableResources wants a map[string]ctlres.Resource
    // it should also be able to use [-1]VersionedResource.Res().
}

type DiffResourcesFactory struct { // or VersionedResourcesFactory?
    // attributes extracted from ChangeSetWithVersionedRs, only used by the extracted methods (only).
    existingRs, newRs []ctlres.Resource
    rules []ctlconf.TemplateRules
    stripNameHashSuffixConfig stripNameHashSuffixConfig
    // extracted attributes end
}

func NewDiffResourcesFactory(exisitingRs, newRs []ctlres.Resource,
    rules []ctlconf.TemplateRule,
    stripNameHashSuffixConfigs []ctlconf.StripNameHashSuffixConfig) DiffResourcesFactory {

    return DiffResourcesFactory{existingRs, newRs,
        rules, stripNameHashSuffixConfigFromConf(stripNameHashSuffixConfigs)}
}

func (d DiffResourcesFactory) NewDiffResources() DiffResources {
    result := DiffResources{}
    result.ExistingResources = d.existingVersionedRs()
    result.ExistingResourcesGrouped = d.existingVersionedRsGrouped()
    result.NewResources = d.newVersionedRs()
    return result
}

// methods extracted from ChangeSetWithVersionedRs

func (d DiffResourcesFactory) existingVersionedRs() versionedResources {
    // existingVersionedResources
}

func (d DiffResourcesFactory) existingVersionedRsGrouped() map[string][]VersionedResource {
    // groupResources / newGroupedVersionedResources
}

func (d DiffResourcesFactory) newVersionedRs() versionedResources {
    // newVersionedResources
}

[100mik]

This seems reasonable to me, but I am not sure if it qualifies as a "Factory".
Worth noting, we do need to ensure that the resources stored in DiffResourcesFactory are updated post renewal of resources, as we would want versioning to be applied to any renewed resources that are versioned

[criztovyl]

True, its not a factory just because it hss a complex constructor... ^^

[100mik]

I am totally for scoping these to a struct. I am wondering if a single struct (just like DiffResourcesFactory) which has functions returning appropriate values would suffice. Especially since most of these are called at most twice.

I do not have a lot of context regarding the Kustomise suffix PR at the moment, but I am definitely for scoping and grouping related functions. If you think this change will benefit the ongoing PR, I am definitely for this!
I am going to take some time to skim over the other PR, meanwhile if you want to go ahead and raise a PR for this change, I would be happy to review it 🚀
(We can figure out what we wanna call things on the PR)

[100mik]

I am yet to go through the other PR, but I am assigning this to you for the time being 🚀