diff internals: Introducing a DiffResource(s) struct
- Dominant language
- Go
- Stars
- 1.1k
- Forks
- 134
- PR merge metrics
- No merged PRs in 30d
Description
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 https://github.com/vmware-tanzu/carvel-kapp/pull/571#discussion_r989791601_
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)](https://github.com/vmware-tanzu/carvel-kapp/pull/571#discussion_r989791601), extracting `existingVersionedResources` and `newVersionedResources` to a new struct.
>
> [...]
>
> ```go
> // 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:
>
> ```go
> 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
> }
> ```
Contributor guide
Assessment
This issue has not been assessed yet.