Skip to content

feat: Add destination when generating sources#1847

Closed
erezrokah wants to merge 2 commits intocloudquery:mainfrom
erezrokah:feat/add_destination
Closed

feat: Add destination when generating sources#1847
erezrokah wants to merge 2 commits intocloudquery:mainfrom
erezrokah:feat/add_destination

Conversation

@erezrokah
Copy link
Copy Markdown
Member

@erezrokah erezrokah commented Sep 18, 2022

Summary

Requires cloudquery/plugin-sdk#120

Somehow fixes #1808.
When generating sources, if there's a destination generated already, add it to the source configuration.

Then I think our docs should first instruct users to generate the destination, then the sources.


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Test locally on your own infrastructure
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

I think this looks good! but I think we have to be cautions with gen turning into a product of itself which generates configs for various use-cases and so on so really important to keep it simple as I would even go far that gen is even not mandatory on it's own as it's quite common in most cases for users to create configs from documentation rather from commands that auto generate configs.

Because for example how do we know the user wants to sync the source with all destinations? so either he will have to add a destination or to delete a destination (not sure which is better I think to explicitly add might be better and the more secure way)

@erezrokah
Copy link
Copy Markdown
Member Author

I think this looks good! but I think we have to be cautions with gen turning into a product of itself which generates configs for various use-cases and so on so really important to keep it simple as I would even go far that gen is even not mandatory on it's own as it's quite common in most cases for users to create configs from documentation rather from commands that auto generate configs.

💯 This is a feature on its own. I'm trying to prevent the back and forth between CLI commands and manually editing files.
Another approach would be to make config creation/editing DX better (i.e. expose schema, have auto completion in IDE), but I think at the moment this is the fastest thing we can do to improve onboarding.

@erezrokah
Copy link
Copy Markdown
Member Author

erezrokah commented Sep 18, 2022

Because for example how do we know the user wants to sync the source with all destinations? so either he will have to add a destination or to delete a destination (not sure which is better I think to explicitly add might be better and the more secure way)

Yes I think this feature might change when we have more destinations (for example we can let the user chose from a list)

@erezrokah
Copy link
Copy Markdown
Member Author

Also waiting on cloudquery/plugin-sdk#120 to merge this

@yevgenypats
Copy link
Copy Markdown
Contributor

Because for example how do we know the user wants to sync the source with all destinations? so either he will have to add a destination or to delete a destination (not sure which is better I think to explicitly add might be better and the more secure way)

Yes I think this feature might change when we have more destinations (for example we can let the user chose from a list)

I wouldn't go into the rabbit hole of adding more features like choosing from a list but rather just output the config and make this feature end at this. Im even at the opinion that the gen command is a bit too much but given we already have it and people like it we can keep it but wouldn't work on adding more features and interactivity to that as it's not the main goal of CQ (other we branch to auto-generating config product)

kodiakhq bot pushed a commit to cloudquery/plugin-sdk that referenced this pull request Sep 18, 2022
Copy link
Copy Markdown
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Actually second thought here - Let's keep the gen command write only without reading the current directly I think it is really important to keep bugs at bay and things like oh it read a different directory and so on. I think the current gen command is way more advanced to the current status of the tool anyway.

@erezrokah
Copy link
Copy Markdown
Member Author

erezrokah commented Sep 18, 2022

Got it, I'll close the PR so we can think this through

@erezrokah erezrokah closed this Sep 18, 2022
@erezrokah erezrokah deleted the feat/add_destination branch September 18, 2022 15: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.

feat(cli-v2): When generating a source the CLI should set at least 1 destination for it

2 participants