Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Singular realizer #857

Merged
merged 4 commits into from
May 18, 2022
Merged

Singular realizer #857

merged 4 commits into from
May 18, 2022

Conversation

squeedee
Copy link
Member

@squeedee squeedee commented May 13, 2022

consolidate much duplication across delivery and supply chain

@idoru idoru force-pushed the singular-realizer branch 4 times, most recently from ddfd0ac to de7c71c Compare May 17, 2022 21:00
@idoru idoru marked this pull request as ready for review May 17, 2022 22:39
"carto.run/template-kind": resource.TemplateRef.Kind,
"carto.run/cluster-template-name": resource.TemplateRef.Name,
}
case *v1alpha1.ClusterDelivery:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that in deliverable_reconciler we're doing

func buildDeliverableResourceLabeler(owner, blueprint client.Object) realizer.ResourceLabeler {
        return func(resource realizer.OwnerResource) templates.Labels {
                return templates.Labels{
                        "carto.run/deliverable-name":      owner.GetName(),
                        "carto.run/deliverable-namespace": owner.GetNamespace(),
                        "carto.run/delivery-name":         blueprint.GetName(),
                        "carto.run/resource-name":         resource.Name,
                        "carto.run/template-kind":         resource.TemplateRef.Kind,
                        "carto.run/cluster-template-name": resource.TemplateRef.Name,
                }
        }
}

while here we're actually doing the switch on the type - is there a particular reason for such difference?

Copy link
Contributor

@cirocosta cirocosta May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for

func MakeSupplychainOwnerResources(supplyChain *v1alpha1.ClusterSupplyChain) []OwnerResource {
	var resources []

we pass straight v1alpha1.ClusterSupplyChain in there - given that build(Deliverable|Workload) already includes the concrete type in the name of the function itself, wdyt of passing *v1alpha1.(deliverable|workload) in the args directly?

(assuming we want to keep the distinct methods)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idoru I dont think we need the switch on type at all? the reconciler is building for Workloads/SCs only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good spot. forgot to remove the switch when we split it.

Copy link
Contributor

@cirocosta cirocosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall lgtm! just some nitpicking
awesome to see the cuts
Screen Shot 2022-05-18 at 11 24 36 AM
!

@idoru idoru force-pushed the singular-realizer branch from 5e4ea99 to 76a172c Compare May 18, 2022 15:54
@idoru idoru enabled auto-merge (squash) May 18, 2022 15:56
@idoru idoru merged commit 4f02212 into main May 18, 2022
@idoru idoru deleted the singular-realizer branch May 18, 2022 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants