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

Move and make utilizable common converters #20

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

sergenyalcin
Copy link
Member

@sergenyalcin sergenyalcin commented Aug 2, 2023

Description of your changes

Depends on: crossplane/upjet#251

This PR:

  • Moves common converters
  • Makes utilizable converters from a standard repo.

Converters previously written in private repos are moved here as reusable components.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@sergenyalcin sergenyalcin force-pushed the common-converters branch 4 times, most recently from 3810e95 to 8e4d34c Compare August 3, 2023 08:37
@sergenyalcin sergenyalcin marked this pull request as ready for review August 3, 2023 08:42
Signed-off-by: Sergen Yalçın <[email protected]>
Copy link
Contributor

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, left some comments to discuss.

converters/common/builder.go Outdated Show resolved Hide resolved
converters/common/builder.go Outdated Show resolved Hide resolved
converters/common/builder.go Outdated Show resolved Hide resolved
converters/common/builder.go Outdated Show resolved Hide resolved
converters/common/builder.go Outdated Show resolved Hide resolved
return errors.Wrap(convertTagsInPatchset(sourcePatchSets, tagsPatchSetName), "failed to convert patch sets")
}

func ConvertPatchesMap(sourceTemplate v1.ComposedTemplate, conversionMap map[string]string) []v1.Patch {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, at a future iteration, we can move this migration source field path to the migration target field path conversion functionality to the default converter implementation (migration.CopyInto) in the core framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Let's handle this in a different iteration. It might be nice to merge this as soon as possible for other stakeholders to use.

converters/common/common.go Outdated Show resolved Hide resolved
converters/template/go.mod Outdated Show resolved Hide resolved
converters/common/common.go Outdated Show resolved Hide resolved
converters/provider-aws/common.go Show resolved Hide resolved
converters/provider-aws/common.go Outdated Show resolved Hide resolved
converters/common/common.go Show resolved Hide resolved
converters/common/common.go Outdated Show resolved Hide resolved
converters/provider-aws/common.go Outdated Show resolved Hide resolved
converters/provider-aws/common.go Show resolved Hide resolved
converters/template/cmd/main.go Show resolved Hide resolved
converters/template/cmd/main.go Outdated Show resolved Hide resolved
converters/template/go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, lgtm.

@sergenyalcin sergenyalcin merged commit eb63a5c into upbound:main Aug 15, 2023
5 checks passed
@sergenyalcin sergenyalcin deleted the common-converters branch August 15, 2023 11:25
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