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

feat(cli): Add validate sub command #1064

Merged
merged 9 commits into from
Oct 24, 2019
Merged

feat(cli): Add validate sub command #1064

merged 9 commits into from
Oct 24, 2019

Conversation

Jeffail
Copy link
Contributor

@Jeffail Jeffail commented Oct 21, 2019

Adds a sub command validate that performs linting of target configs.

The syntax for this subcommand is vector -c ./foo.toml validate, which is potentially a little confusing. Please refer to discussion within #98.

The syntax for this subcommand is `vector -c ./foo.toml validate`, which
is potentially a little confusing. Please refer to discussion within #98.

We also need to separate out the topology validation so that data
directory checks are done separately.

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
The logic for walking and verifying a topology config has been extracted
and moved into its own function. This allows the validate subcommand to
check a config topology without attempting to create the underlying
components.

The checks for >0 sources and >0 sinks have also been moved from config
parsing into topology building. The reason for this is to accomodate
config parsing and validation of snippets (which might only contain
partial topologies) in future work.

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@binarylogic binarylogic changed the title feature(cli): Add validate sub command feat(cli): Add validate sub command Oct 21, 2019
src/main.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looking great!

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
For the betterment of society this commit allows users to specify the
target config path of the validate command after the command itself,
e.g. `vector validate -c ./foo.toml`. In order for that to be safe we
need to deny users the ability to specify the path at the root (as it
will be ignored).

To solve this I've added a check at the beginning of the command to
ensure that the root config flag is not default. This solution is pretty
gross so I'm hoping there's a more elegant way to do this.

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@Jeffail
Copy link
Contributor Author

Jeffail commented Oct 22, 2019

For the betterment of society I've added a commit that allows users to specify the target config path of the validate command after the command itself, e.g. vector validate -c ./foo.toml. In order for that to be safe we need to deny users the ability to specify the path at the root (as it
will be ignored).

To solve this I've added a check at the beginning of the command to ensure that the root config flag is not default.

This solution is pretty gross as we have to copy/paste our default config path several times and ensure they have parity. Ideally I would like to have a const defined with this path, but it doesn't appear as though you can reference a variable within the StructOpt attribute.

Alternatively, maybe we could reference the default_value attribute of the struct field within the validate implementation, but I'm not sure that's possible. Help me.

@Jeffail Jeffail mentioned this pull request Oct 22, 2019
Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@LucioFranco
Copy link
Contributor

@Jeffail I think your approach is looking great, let me know once this is out of a draft PR and ill give it a final review :) if you have specific things you'd like me to look at let me know.

@Jeffail Jeffail marked this pull request as ready for review October 22, 2019 17:48
@Jeffail
Copy link
Contributor Author

Jeffail commented Oct 22, 2019

@LucioFranco thanks, I'd say the only thing left outstanding is whether you have an idea as to how the strings defined https://github.com/timberio/vector/pull/1064/files#diff-639fbc4ef05b315af92b4d836c31b023R101 and https://github.com/timberio/vector/pull/1064/files#diff-639fbc4ef05b315af92b4d836c31b023R369 could be forced to have parity.

My worry is that one day we might choose to change the default path but forget to update the check within validate. It's perhaps not likely, and the future make check-examples command would break in that case (once I've merged it) so we'd at least have some way of capturing it in our CI, so if there's no clear way of doing it then I'm happy to move on.

src/main.rs Outdated Show resolved Hide resolved
Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@Jeffail
Copy link
Contributor Author

Jeffail commented Oct 22, 2019

@LucioFranco think I'm ready for a final review now.

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Well I thought I approved this already...but I guess it didn't go through but LGTM!

@binarylogic
Copy link
Contributor

@Jeffail do you want to attempt to add documentation for this? I think it would make sense here:

https://docs.vector.dev/usage/administration/validating

Signed-off-by: Ashley Jeffs <ash@jeffail.uk>
@Jeffail Jeffail merged commit 018db5f into master Oct 24, 2019
@Jeffail Jeffail deleted the add-validate-subcmd branch October 24, 2019 08:02
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