Skip to content

feat: add extensible transform API for custom field transformations #808

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

Merged
merged 1 commit into from
Jul 23, 2025

Conversation

OrlovEvgeny
Copy link
Contributor

@OrlovEvgeny OrlovEvgeny commented Jul 14, 2025

This enables users to extend compose-go with custom protocols and validation logic without forking the project, addressing the need for custom port protocols like http/https and domain name support

  • Add RegisterDefaultValue() functions
  • Add EnhancedPortDefaults as example of extensible port transformation
  • Maintain backward compatibility with existing code

example:

// EnhancedPortDefaults is an example of an enhanced port defaults function
// that supports additional protocols and domain names in the published field.
// This function can be used as a replacement for the default portDefaults
// by registering it with RegisterDefaultValue.
//
// Example usage:
//   transform.RegisterDefaultValue("services.*.ports.*", transform.EnhancedPortDefaults)
//
// This function supports:
// - Additional protocols: "http", "https", "tcp", "udp"
// - Domain names in the published field (e.g., "example.com:80")
// - All existing functionality of the original portDefaults
func EnhancedPortDefaults(data any, _ tree.Path, _ bool) (any, error) {
	switch v := data.(type) {
	case map[string]any:
		// Set default protocol if not specified
		if _, ok := v["protocol"]; !ok {
			v["protocol"] = "tcp"
		}
		// Set default mode if not specified
		if _, ok := v["mode"]; !ok {
			v["mode"] = "ingress"
		}
		// Additional logic can be added here to handle domain names
		// in the published field or other custom transformations
		return v, nil
	default:
		return data, nil
	}
}

Related issue: psviderski/uncloud#44

@OrlovEvgeny OrlovEvgeny requested a review from ndeloof as a code owner July 14, 2025 10:46
@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch from b916851 to ec2c08a Compare July 14, 2025 10:55
var DefaultValues = map[tree.Path]TransformFunc{}

// For backward compatibility
var defaultValues = DefaultValues
Copy link
Collaborator

Choose a reason for hiding this comment

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

not needed, as a non-public field there's no backward compatibility to be addressed here

@@ -30,6 +34,20 @@ func init() {
defaultValues["services.*.gpus.*"] = deviceRequestDefaults
}

// RegisterDefaultTransformer registers a custom transformer for the given path pattern
func RegisterDefaultTransformer(path string, transformer TransformFunc) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

RegisterDefaultTransformer sounds weird to me. What about RegisterDefaultValue

}

// GetDefaultTransformers returns a copy of the current default transformers
func GetDefaultTransformers() map[string]TransformFunc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why have this function while DefaultValues is public ?

@ndeloof
Copy link
Collaborator

ndeloof commented Jul 15, 2025

Can you please squash your commits, rebase and sign-off, so we can merge ?

@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch 3 times, most recently from af372c9 to 6bf03b7 Compare July 15, 2025 09:33
@OrlovEvgeny
Copy link
Contributor Author

Hey @ndeloof

Can you please squash your commits, rebase and sign-off, so we can merge ?
Done

@ndeloof
Copy link
Collaborator

ndeloof commented Jul 15, 2025

seems you included way more than the expected changes in your commit :')

@OrlovEvgeny
Copy link
Contributor Author

I see,It seems something broke after I rebased main
let me fix it

@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch 4 times, most recently from d981d63 to de3bbc1 Compare July 15, 2025 11:59
@OrlovEvgeny
Copy link
Contributor Author

@ndeloof fixed

@OrlovEvgeny OrlovEvgeny requested a review from ndeloof July 15, 2025 12:02
// TransformFunc is a function that can transform data at a specific path
type TransformFunc func(data any, p tree.Path, ignoreParseError bool) (any, error)

// For backward compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch from de3bbc1 to dade8b5 Compare July 15, 2025 12:44
@ndeloof ndeloof enabled auto-merge (rebase) July 15, 2025 13:22
auto-merge was automatically disabled July 15, 2025 13:24

Head branch was pushed to by a user without write access

@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch 2 times, most recently from c153775 to cc0c190 Compare July 15, 2025 13:39
@OrlovEvgeny
Copy link
Contributor Author

Linter complains about "stuttering" when using type as transform.TransformFunc
https://github.com/compose-spec/compose-go/actions/runs/16293608125/job/46012331031#step:4:33

I refactored some naming:
renamed TransformFunc to Func in transform/canonical.go
what do you think about it?

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

Sounds good!
I wonder if we should not add a test, to avoid breaking change on the feature and give an exemple for developers who want to use it, WDYT?

@OrlovEvgeny OrlovEvgeny force-pushed the feat/extensible-transform-api branch 2 times, most recently from c22999a to 79a2dfe Compare July 15, 2025 17:04
@OrlovEvgeny
Copy link
Contributor Author

Sounds good! I wonder if we should not add a test, to avoid breaking change on the feature and give an exemple for developers who want to use it, WDYT?

Yes, I think that's a good idea

I added a test and an example function exampleEnhancedPortDefaults

@OrlovEvgeny OrlovEvgeny requested a review from glours July 15, 2025 17:06
@OrlovEvgeny OrlovEvgeny requested a review from ndeloof July 21, 2025 10:32
@OrlovEvgeny
Copy link
Contributor Author

Hi team @glours @ndeloof , just wanted to check if you expect me to make any more changes here?

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for this contribution 🙏

@glours glours enabled auto-merge (rebase) July 23, 2025 09:04
@glours glours force-pushed the feat/extensible-transform-api branch from dd88e9a to 3aed6c5 Compare July 23, 2025 09:04
@glours glours merged commit 6d8281c into compose-spec:main Jul 23, 2025
8 checks passed
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