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

[Merged by Bors] - Add fluvio connector config to cli to get connector configs. #2464

Closed

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Jul 5, 2022

Closes #2458.

Notes from discussion with @ajhunyady: add json output type option.

We chose to move to using fluvio connector config <name> [-o output-file]

@simlay simlay marked this pull request as ready for review July 5, 2022 17:52
@simlay simlay force-pushed the fluvio-connector-describe-command branch from 3333335 to 32549ef Compare July 5, 2022 18:53
@simlay simlay requested review from sehz and tjtelan July 5, 2022 18:53
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

Generally looks good. Unfortunately, I think we don't seem to have an existing framework for describe which we should start

crates/fluvio-cli/src/connector/describe.rs Outdated Show resolved Hide resolved
return Err(CliError::ConnectorNotFound(connector_name));
};
let connector: ConnectorConfig = connector.into();
let connector = serde_yaml::to_string(&connector)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should support all output types right? (YAML,JSON, Text) We should probably have trait for it so can standardize it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do actually kinda already have this. If you were to do fluvio connector list -O [json|yaml] you'll get the connectors spec as json/yaml. The issue is that it's pretty much just the CRD as json or yaml. Not the connector yaml which the user will want to copy and paste for a fluvio connector create or fluvio connector update command.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I try to make use of this it? I'm sure we'll want this for stuff like for fluvio table-format.

Copy link
Contributor

@tjtelan tjtelan left a comment

Choose a reason for hiding this comment

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

Can we have a CLI test that starts a connector and just runs the describe command and tests that it exits appropriately?

@simlay simlay changed the title Connector describe Add fluvio connector config to cli to get connector configs. Jul 7, 2022
@simlay simlay force-pushed the fluvio-connector-describe-command branch from 059e947 to 464b7a9 Compare July 7, 2022 16:35
@simlay simlay requested review from tjtelan and sehz July 7, 2022 17:29
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

Looks good. a very minor nit.

tests/cli/smoke_tests/connector-describe.bats Outdated Show resolved Hide resolved
Copy link
Contributor

@sehz sehz left a comment

Choose a reason for hiding this comment

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

LGTM. Good work!

@simlay
Copy link
Contributor Author

simlay commented Jul 7, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 7, 2022
Closes #2458.

~Notes from discussion with @ajhunyady: add json output type option.~

We chose to move to using `fluvio connector config <name> [-o output-file]`
@bors
Copy link

bors bot commented Jul 7, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Add fluvio connector config to cli to get connector configs. [Merged by Bors] - Add fluvio connector config to cli to get connector configs. Jul 7, 2022
@bors bors bot closed this Jul 7, 2022
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.

Add connector describe command
3 participants