Skip to content

feat(cli): Add scaffold command for destination#2055

Closed
yevgenypats wants to merge 5 commits intomainfrom
feat/cli/scaffold
Closed

feat(cli): Add scaffold command for destination#2055
yevgenypats wants to merge 5 commits intomainfrom
feat/cli/scaffold

Conversation

@yevgenypats
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats commented Sep 25, 2022

source will follow shortly.

This will enable us to deprecate https://github.com/cloudquery/cq-provider-template and update v2 documentation accordingly

source will follow shortly.

This will enable us to depreciate https://github.com/cloudquery/cq-provider-template
and update v2 documentation accordingly
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Since scaffolding can turn into its own product, I tend not to put in the CLI in favor of a separate tool. We need to distinguish between users that consume the data (using the CLI) and plugin authors.

Also I think this is something we can do post v1 and remove the current docs about developing new plugins.

The current community plugins were developed tightly with the CloudQuery team anyways (correct me if I'm wrong), so maybe we can put this on hold for now pending a further discussion on how we want the plugin author experience to look like and also discuss our approach with the current authors.

Please let me know wdyt and maybe we can have a discussion with the rest of the team too

@yevgenypats
Copy link
Copy Markdown
Contributor Author

I Agree 100% that it might turn into it's own product but have the following caveats:

  • I don't think we want to support full blown scaffold in the next 12 months
  • Scaffolding is just the UI layer while the cornerstone for new plugins in developers experience and ease of contribution is what we worked on in the last 3 months: Idiomatic Go API, simple logging with zerolog, standard error reporting, simple grpc API without go-plugin and codegen via SDK and not HCL (prob missed something). Given we did all that and we had contributions before I dont think we should hide our developer guide but rather just update it.
  • Scaffolding is here just a quick replacement for https://github.com/cloudquery/cq-provider-template. I don't think we want to maintain now a full blown scaffold so I think starting from a command makes sense. If we realise it too much of maintaince we can either drop it and just keep documentation or turn it into a scaffold product if our external/community plugin contribution skyrocket.

So to sum up I think we did enough improvement to the developer experience and we should invest more but rather just finalize it with this simple command and documentation and do it step by step rather then jumping straight ahead to full blown scaffold product when we dont have much contributors to external plugins. This will also give us a relatively easy way of getting operational experiance and feedback about scaffolding for a scaffold project if such will become a thing in the future.

@erezrokah
Copy link
Copy Markdown
Member

Thanks for the additional context. I should have been more clear with my suggestion.
I'm not suggesting to create a separate CLI, but to have a doc that says - if you want to create a plugin see our GCP one as an example and also reach out to us on Discord.
I'm sure there are plenty of gotchas when creating a plugin. Some of those might be solved via scaffolding, some via SDK improvements and some by solutions we have not found yet.
My concern is that we've reduced the complexity of the CLI to a single command which is the most important one (sync), so adding a new command that might not be useful for most consumers can be confusing for those.
For example, we've removed the 'gen' command to reduce complexity, which I believe is more useful to data consumers than scaffold.

With all that being said, I'd rather see us ship fast and ship to learn, as we can always do a breaking change to remove commands if we'd like, just wanted to share my 2 cents.

@yevgenypats
Copy link
Copy Markdown
Contributor Author

100% agree that in the docs we should say take a look at the gcp plugin, sdk codegen, talk to us on discord and so on. The working plugins are the most useful place to learn - no doubt about that.

We removed the gen command because we couldn't figure a way where our documentation will be consistent and we understood it is a rabbit hole of maintaining duplicates documentation and heavy for plugin devellopers as well (cloudquery/plugin-sdk#169).

I do think we missed the very good point that you brought up about figuring the latest version and Im going to try and bring back gen command with only this capability.

}
}

func fetchSampleTable(ctx context.Context, meta schema.ClientMeta, parent *schema.Resource, res chan<- interface{}) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should re-enforce the convention of placing fetch functions inside separate _fetch.go files here

func (c *Client) Logger() *zerolog.Logger {
return &logger
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the Client is missing a New function (referenced in main.go)


import (
"github.com/cloudquery/plugin-sdk/plugins"
"github.com/{{.Org}}/cq-source-{{.Name}}/client"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing github.com/cloudquery/plugin-sdk/schema import here

@@ -0,0 +1,16 @@
package client

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing imports section:

import (
	"context"
	"fmt"
	"github.com/cloudquery/plugin-sdk/schema"
	"github.com/cloudquery/plugin-sdk/specs"
	"github.com/rs/zerolog"
)

@hermanschaaf
Copy link
Copy Markdown
Contributor

I think I agree with @erezrokah that I'd rather wait for post-v1 with this functionality, but at the same time I do also agree that having something is better than having nothing. As long as we're okay with making breaking changes to this command (including maybe removing it down the line) I'm happy to go with it.

That all said, the implementation looks like a great start; a couple of things I'll add:

  • we should also generate a go.mod (and maybe go.sum) file, so that the code compiles immediately after running the scaffold command. Most reliable way for us to do this is likely to run go mod init <path> and go mod tidy in the directory after it was generated, but that won't work if the user doesn't have go installed. If we don't do this, we should at least include these steps in the README.
  • (probably not for this iteration) we need to think about how we're going to support changes to this template made after a plugin was generated. https://github.com/cruft/cruft is an example of a tool that has a solution for this; basically it stores the last-used commit as metadata and then runs a diff when you update it, making updates semi-automatic. We can use a similar process, or come up with something else, but this is important to think about.
  • (probably not for this iteration) including some examples of using the SDK to do codegen might be useful

yevgenypats and others added 3 commits September 26, 2022 14:52
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
@yevgenypats
Copy link
Copy Markdown
Contributor Author

yevgenypats commented Sep 26, 2022

ok I agree 🙃 . Let's skip this for now.

@hermanschaaf hermanschaaf deleted the feat/cli/scaffold branch January 10, 2023 09:37
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.

4 participants