Skip to content

feat: Write to config file instead of stdout (v2)#1645

Merged
yevgenypats merged 8 commits intocloudquery:feat/cloudquery-cli-v2from
hermanschaaf:feat/cloudquery-cli-v2-output-file
Sep 1, 2022
Merged

feat: Write to config file instead of stdout (v2)#1645
yevgenypats merged 8 commits intocloudquery:feat/cloudquery-cli-v2from
hermanschaaf:feat/cloudquery-cli-v2-output-file

Conversation

@hermanschaaf
Copy link
Copy Markdown
Contributor

@hermanschaaf hermanschaaf commented Aug 31, 2022

Changes the default behavior of generate to write to file, rather than stdout. Also adds an --output (or -O) flag to specify where output should be written. Stdout output is currently explicitly not supported, to keep the featureset minimal.

This depends on cloudquery/plugin-sdk#29 for getting the plugin name, so go.mod will need to be updated here after that is merged in and released.

@hermanschaaf hermanschaaf changed the title feat: Write to config file instead of stdout feat: Write to config file instead of stdout (v2) Sep 1, 2022
}
var columnName pgx.Identifier = []string{c.Name}
fieldDef := columnName.Sanitize() + " " + pgType
if c.CreationOptions.Unique {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@yevgenypats I had to make some changes to this file to handle the Unique option not being there anymore (so the code could compile). Could you double-check this please?

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.

Im pushing a pr that removes this atm.

@hermanschaaf hermanschaaf marked this pull request as ready for review September 1, 2022 08:59
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.

few questions and also the postgresql code it will create a conflict for me as Im removing the unique atm and pushing a pr as well

if err != nil {
return errors.Wrap(err, "failed to write file")
}
fmt.Fprintln(os.Stderr, "✅ Source plugin config written to "+configPath)
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.

no emojies please :) otherwise we will have to introduce more flags like no color to stdout and so forth.
also why stderr if it's all prints it successfully ?

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.

also let's use fmt.Errorf - I was experimenting with errors.Wrap to try and get the line number for sentry but it doesn't work great - I think we should only use it in plugins where it's hard to know where the error came from.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

stderr because this is a log line and shouldn't be seen as the program's output (unless that is what we want?). Usually log lines go to stderr. At the same time, I didn't want the log line going to the log file, because then the CLI user doesn't see it unless they pass a special flag (that's why I didn't use log.Info() here either).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll take the emoji out 😢

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 just output to the user (we can also write to the log in addition)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

stderr is shown to the user, and I think it's the right place for something that is not strictly part of the program's output and is subject to change (like how Go's log package also writes to stderr). I'd like to keep it stderr, since I think that's better/more correct, but will change if you think we should still use stdout?

}
fmt.Println(res)

configPath := name + ".yml"
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.

let's just ask the user to pass something ending with yml otherwise it will be .yml.yml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is for when the user hasn't passed in the flag (the name comes from the plugin RPC call)

if err != nil {
t.Fatalf("error reading config file output: %v ", err)
}
err = yaml.Unmarshal(yamlFile, &cfg)
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.

Can we do even further check on the content? just string equality I suggest?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done 👍

}
var columnName pgx.Identifier = []string{c.Name}
fieldDef := columnName.Sanitize() + " " + pgType
if c.CreationOptions.Unique {
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.

Im pushing a pr that removes this atm.

@hermanschaaf hermanschaaf added the automerge Automatically merge once required checks pass label Sep 1, 2022
@yevgenypats yevgenypats merged commit d2352f1 into cloudquery:feat/cloudquery-cli-v2 Sep 1, 2022
yevgenypats pushed a commit that referenced this pull request Sep 6, 2022
* feat: Write to config file instead of stdout

* Update SDK to v0.0.10 and remove unique constraints

* Add more assertions to generate test

* Write to stdout

* clarify if statement
yevgenypats added a commit that referenced this pull request Sep 6, 2022
* feat!: CloudQuery V2.

goes together with cloudquery/plugin-sdk#4

This is V2 of CloudQuery CLI together with the new https://github.com/cloudquery/plugin-sdk

- Support for multiple databases. plugins are sending info back to CLI
- seperate CLI configuration from plugins configuration.

- Moved to plain gRPC (no more go-plugin)
- zerolog everwhere

- no custom UI implementations
- no bloated/unneeded abstraction layers

Issues that should be fixed:

#1104
#1055
#983
#859
#858
#888
#857
#854
#904
#751
#539
#392
#292
#159
#908
#840

* migrate gcp to new sdk

* work on a new abstraction layer for codegen

* re-implement cq-gen

* Add autogenerated mock tests

* auto generated compute service

* working compute service

* regenerated gcp with compute with primary key

* Add automigrate to postgresql-dest

* small logging fix

* improve pg tests

* more work

* pg storage working with upsert

* wip

* Compiled resources

* autogenerated test passing

* remove old gcp services

* remove more old services

* migrate more gcp services

* PostgreSQL fixes

* fix pg migration

* working on more tests

* remove old gcp workflows

* more tests

* Adding more tests

* rebase more or less worked

* remove replace directive

* more go lint fixes

* update plugin-sdk

* make gcp compile

* fix more linters

* fix race condition bug with plugin hanging

* bugfix in community plugins

* fix plugin tests

* fix: Show help on parsing errors

* fix review

* feat: Remove unique from PostgreSQL destination

* remove the need for deprecated errors package

* feat: Write to config file instead of stdout (v2) (#1645)

* feat: Write to config file instead of stdout

* Update SDK to v0.0.10 and remove unique constraints

* Add more assertions to generate test

* Write to stdout

* clarify if statement

* fix: pgx_log_level not mandatory

* feat: Add errors/warnings summary

* test: Add more cmd tests (#1696)

test: Add more tests to cli

* chore: Remove old test file

* feat: Add example of list/detail codegen (#1706)

* feat: Add example of list/detail codegen

* make naming more reasonable

* finalizing gcp

* finalize gcp migration

* feat: New go sdk for gcp (#1720)

* update to latest sdk

* feat: Move to new SDK

* more fixes to autogeneration

* more fixes to gcp

* feat: Add sentry

* remove old faker

* go mod tidy

* fix cli tests

Co-authored-by: Herman Schaaf <hermanschaaf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants