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

Specgen #198

Merged
merged 14 commits into from
Nov 15, 2024
Merged

Specgen #198

merged 14 commits into from
Nov 15, 2024

Conversation

lovromazgon
Copy link
Member

@lovromazgon lovromazgon commented Nov 6, 2024

Description

Depends on ConduitIO/conduit-commons#131 and https://github.com/ConduitIO/evolviconf (we should clean up that repo and release it so we depend on a released tag).

This contains the new specgen tool which extracts the connector specification into a YAML file. It should always be in the root of the repository and called connector.yaml (or connector.yml).

Usage

After this PR is merged and released, the developer should be able to install the tool using:

go install github.com/conduitio/conduit-connector-sdk/specgen

It should be added in a connector repository in the tools.go file as an import instead of paramgen:

  import (
- 	_ "github.com/conduitio/conduit-commons/paramgen"
+ 	_ "github.com/conduitio/conduit-connector-sdk/specgen"
  )

In the connector.go file it should be added in a go:generate directive, it shouldn't need any flags, just simply:

//go:generate specgen

When upgrading an existing connector to use specgen, you should update the code according to the code changes described in the chapter below, and then generate the initial connector.yaml so that the existing specification is copied over. After that, you can delete the spec.go file and change the connector.go file according to this:

//go:embed connector.yaml
var specs string

var Connector = sdk.Connector{
	NewSpecification: sdk.YAMLSpecification(specs),
	NewSource:        NewSource,
	NewDestination:   NewDestination,
}

This will ensure that the specifications are read from connector.yaml, making it the source of truth for the connector specifications.

Code changes in connectors

This PR changes a few things in the SDK, which will make it necessary to update the connector code. The good news is that it actually simplifies things.

No more Parameters

Neither the Source nor Destination interfaces require the Parameters method anymore. In fact, they were completely removed and are not used anymore. Connectors are expected to fetch the parameter specifications from the YAML file instead.

No more Configure

The Configure method was removed from Source and Destination. This simplifies the interfaces and does not require explicit parsing of the config anymore. Instead, the SDK will take care of parsing the config returned from the interfaces in Config.

Introduced Config

Config replaces both Parameters and Configure in Source and Destination. The connector should return the struct representing the connector configuration as a pointer. The pointer is required so that the SDK can parse the configuration map into that value, making the parsed configuration visible to the connector.

The returned config struct needs to implement SourceConfig / DestinationConfig. Both look the same, they allow the config (but don't require) to implement a method Validate. This will be called by the SDK after parsing the configuration, in case there are any further manual validations that need to happen. This essentially replaces Configure and makes it clear that it should only execute validations.

Note that the defined config struct should normally embed sdk.DefaultSourceMiddleware / sdk.DefaultDestinationMiddleware, so that the middleware is attached to the connector:

type MyConfig struct {
	sdk.DefaultSourceMiddleware
	// other fields ...
}

Another improvement is that the developer can't really "forget" to add the middleware, since the config needs to embed sdk.UnimplementedSourceConfig / sdk.UnimplementedDestinationConfig, not doing so will result in a compile-time error. So they need to either embed it directly (resulting in no middleware) or embed the default middleware struct mentioned above, which already includes the unimplemented config.

Middleware

Source/destination middleware implementations were drastically simplified because of these changes. The middleware is now essentially just a config struct, which needs to be embedded in the connector config. The parameters are automatically parsed when parsing the config, since they are pointers to fields in the config struct defined in the connector. This means that the middleware now doesn't have to manually implement Parameters and Configure, it is all done using specgen and the SDK.

The default middleware is defined in sdk.DefaultSourceMiddleware / sdk.DefaultDestinationMiddleware, which needs to be embedded by the config struct defined in the connector. That config struct now dictates which middleware will wrap the source/destination. If the developer wants to overwrite any defaults in the middleware, they can do so by setting the corresponding values when instantiating the connector:

type Source struct {
	sdk.UnimplementedSource

	config MyConfig
}

func NewSource() sdk.Source {
	return sdk.SourceWithMiddleware( // Extract the middleware from the config and wrap the source
		&Source{
			config: MyConfig{
				DefaultSourceMiddleware: sdk.DefaultSourceMiddleware{ // Change defaults in the default middleware
					SourceWithSchemaExtraction: sdk.SourceWithSchemaExtraction{
						PayloadEnabled: lang.Ptr(false), // Set default value to false
						KeyEnabled:     lang.Ptr(false), // Set default value to false
					},
				},
			},
		},
	)
}

// Config returns a pointer to the config struct. Values will be automatically populated by the SDK.
func (s *Source) Config() sdk.SourceConfig {
	return &s.config
}

Normally, when the developer doesn't need to overwrite any defaults, this can be simplified:

func NewSource() sdk.Source {
	return sdk.SourceWithMiddleware(&Source{})
}

Open work

What's still missing in this PR:

  • Rename connector field in the YAML file to specification. Will be done in specgen: add more tests, rename connector to specification #206.
  • specgen: Order fields (required first, then by name alphabetically) #209
  • Update the validation function to use struct tags instead of requiring the parameter specifications (NOTE: this part needs to be done in conduit-commons and will affect processors as well, although positively).
    • Kept it simple for now and used the same validation strategy we used before (using parameters from the specification), only that it's executed in the SDK. We can change this in the future, since this is now an internal thing in the SDK.
  • Make sure that Validate is called on SourceConfig / DestinationConfig and make sure that the same method is called for all middleware configs as well.
    • Kept this simple as well, if the developer overwrites the Validate method in their own connector they need to call the middleware Validation method as part of the implementation. It's called out in the documentation.
  • Acceptance tests need to be uncommented and adjusted, same goes for the benchmark and probably a lot of the test files.
  • Tests, a lot of tests...

Related to ConduitIO/conduit#1523.

Related issues:

Quick checks:

  • I have followed the Code Guidelines.
  • There is no other pull request for the same update/change.
  • I have written unit tests.
  • I have made sure that the PR is of reasonable size and can be easily reviewed.

@hariso hariso changed the base branch from main to specgen November 15, 2024 09:43
Copy link
Member

@raulb raulb left a comment

Choose a reason for hiding this comment

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

Everything I would comment on before considering this in a state other than draft is already tracked on ConduitIO/conduit#1523, so I'm good with making this part of the feature branch so we can iterate on the main feature independently.

destination.go Show resolved Hide resolved
@hariso hariso marked this pull request as ready for review November 15, 2024 14:05
@hariso hariso requested a review from a team as a code owner November 15, 2024 14:05
@hariso hariso merged commit 07483f5 into specgen Nov 15, 2024
2 of 3 checks passed
@hariso hariso deleted the lovro/specgen-spike branch November 15, 2024 14:06
hariso added a commit that referenced this pull request Nov 28, 2024
Date:   Thu Nov 28 12:44:48 2024 +0100

    add open

commit 2705320
Author: Haris Osmanagic <[email protected]>
Date:   Thu Nov 28 11:21:36 2024 +0100

    destination_middleware_test.go

commit b3f8172
Author: Haris Osmanagic <[email protected]>
Date:   Thu Nov 28 10:08:22 2024 +0100

    fix source_test

commit 6a53d24
Author: Haris Osmanagic <[email protected]>
Date:   Thu Nov 28 10:02:23 2024 +0100

    fix source middleware

commit 6cf5795
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 27 15:39:46 2024 +0100

    add test

commit be36d59
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 16:01:29 2024 +0100

    fix source_middleware_test.go

commit abdb4b7
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:54:48 2024 +0100

    fix DefaultDestinationMiddleware

commit b07a4b5
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:50:00 2024 +0100

    linter

commit 727489f
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:49:46 2024 +0100

    comments

commit 768c935
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:46:33 2024 +0100

    add comment

commit d9f7f64
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:42:57 2024 +0100

    fix test expectation

commit 9f5c6c2
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 12:35:06 2024 +0100

    handle schema type

commit ddd70f5
Author: Haris Osmanagic <[email protected]>
Date:   Wed Nov 20 11:03:34 2024 +0100

    workaround for schema type, optional dest mw param, fix validation

commit fe751ca
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 15:17:30 2024 +0100

    restructure

commit 7318263
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 14:59:25 2024 +0100

    fix tests

commit 04e9fd4
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 14:06:01 2024 +0100

    re-enable acceptance tests

commit c5174a6
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 13:08:09 2024 +0100

    partial specs test

commit dcc08ed
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 12:48:02 2024 +0100

    write yaml, overwrite existiang source and destination

commit 8af981f
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 12:45:18 2024 +0100

    more tests, restructure

commit 1755bac
Author: Haris Osmanagic <[email protected]>
Date:   Tue Nov 19 12:37:09 2024 +0100

    revert changes to input file

commit dabaf2e
Author: Haris Osmanagic <[email protected]>
Date:   Mon Nov 18 19:12:42 2024 +0100

    update test

commit 2b4394a
Author: Haris Osmanagic <[email protected]>
Date:   Mon Nov 18 19:09:57 2024 +0100

    simplify

commit ab77011
Author: Haris Osmanagic <[email protected]>
Date:   Mon Nov 18 19:08:12 2024 +0100

    TestWriteAndCombine

commit f51b771
Author: Haris Osmanagić <[email protected]>
Date:   Mon Nov 18 18:22:24 2024 +0100

    specgen: add more tests, rename connector to specification (#206)

commit 07483f5
Author: Lovro Mažgon <[email protected]>
Date:   Fri Nov 15 15:06:29 2024 +0100

    Specgen (#198)

    * specgen spike

    * start nicer rewrite of specgen

    * support for parsing builtin params

    * support for time.Duration

    * support types in different packages

    * support underlying type

    * better tests

    * specgen specification parsing and default overwriting

    * add support for destinations

    * fix test

    * add parsing and validation of config

    * adjust most destination tests

    ---------

    Co-authored-by: Haris Osmanagic <[email protected]>
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