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

[Composition] Adding string transform ToUpper/ToLower #2592

Merged
merged 5 commits into from
Oct 15, 2021

Conversation

mcbenjemaa
Copy link
Member

Description of your changes

Adding 2 fields to the StringTransform, in order to make it possible to add extra functionally of the string transformation.
In our case we need to add a convert ToUpper/ToLower.

  • string.type field is required and so, every time must be set! (breaking change!!) any thoughts ?
  • string.fmt used to format string as usual.
  • string.convert accepts 2 values ToUpper, ToLower, will convert the string according to it's value.
type StringTransform struct {
	Type StringTransformType `json:"type"`
	Format *string `json:"fmt,omitempty"`
	Convert *StringConversionType `json:"convert,omitempty"`
}

Fixes #2579

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

cc @negz
/label "backport release-1.5"

How has this code been tested

Tested locally using kind cluster.

Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

A few nits, but in general this looks great. Thank you!

apis/apiextensions/v1/composition_transforms.go Outdated Show resolved Hide resolved
@mcbenjemaa mcbenjemaa force-pushed the feature/string-upper-lower branch 2 times, most recently from 4886b43 to c406d0c Compare September 27, 2021 11:11
negz
negz previously approved these changes Oct 11, 2021
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Two nits and a question that would be nice to fix before we merge, but mostly this looks great. Thanks @mcbenjemaa!

apis/apiextensions/v1/composition_transforms.go Outdated Show resolved Hide resolved
apis/apiextensions/v1/composition_transforms.go Outdated Show resolved Hide resolved
Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>
Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>
…tringTransform to v1beta1

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>
@mcbenjemaa mcbenjemaa force-pushed the feature/string-upper-lower branch 2 times, most recently from ce40ba3 to 172e07a Compare October 12, 2021 18:21
Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>
@negz
Copy link
Member

negz commented Oct 13, 2021

Thank you for working on this @mcbenjemaa! One final thing - could you add a section for this new transform to the docs? Apologies that I didn't think to mention this earlier.

@negz negz dismissed their stale review October 13, 2021 20:08

Code looks good but I'd like to wait for docs too.

Signed-off-by: Mohamed Chiheb <mc.benjemaa@gmail.com>
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.

ToUpper/ToLower Composition string transform
2 participants