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

add -parse-only option #1545

Merged
merged 4 commits into from
Mar 15, 2022
Merged

add -parse-only option #1545

merged 4 commits into from
Mar 15, 2022

Conversation

jrwren
Copy link
Contributor

@jrwren jrwren commented Feb 25, 2022

Allows checking of templates for well formedness without rendering.

@jrwren jrwren requested a review from a team February 25, 2022 20:14
@hashicorp-cla
Copy link

hashicorp-cla commented Feb 25, 2022

CLA assistant check
All committers have signed the CLA.

Allows checking of templates for well formedness without rendering.
@eikenb
Copy link
Contributor

eikenb commented Mar 1, 2022

Hey @jrwren, thanks for the PR!

It looks like you based this around the Once flag which was perfect as this would be anther command line only option. It is missing a few things though, mainly tests. There should be tests for the CLI and config which would follow the same model as Once still. The part from runner would be a bit more involved, but not to bad I think. It would need to test running Start() and the appropriate functionality. There is a once test in there as well and it might not make to bad of a base case. Please feel free to ask if you have any questions. Thanks again for submitting this.

@jrwren
Copy link
Contributor Author

jrwren commented Mar 3, 2022

I took a stab at writing some tests, but the tests on master don't run at all for me on my mac.

$ go test -vv .
exit status 1
FAIL	github.com/hashicorp/consul-template	0.265s

No error messages or text output, just exit status 1.

So I added what I consider to be a pointless test to cli_test and hope CI passes.

@eikenb
Copy link
Contributor

eikenb commented Mar 7, 2022

Hey @jrwren,

I enabled CI tests and things look good there.

To test try go test -v ./... and that should run all the tests.

@jrwren
Copy link
Contributor Author

jrwren commented Mar 8, 2022

When I run go test -v ./... I get many errors, seemingly unrelated to the code I wrote, but now that I look a little deeper, I see it is only TestVaultConfig_Finalize.

Can you suggest more areas to test or is this ready?

Copy link
Contributor

@eikenb eikenb left a comment

Choose a reason for hiding this comment

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

These are pretty pointless, but they are there and, until possibly removed, I prefer to keep them complete. Thanks.

@eikenb
Copy link
Contributor

eikenb commented Mar 9, 2022

The tests all pass for me locally and in CI so I'm not sure what issue you're seeing. If you want to include the output of the failing test maybe I can help.

@eikenb
Copy link
Contributor

eikenb commented Mar 9, 2022

Hey @jrwren, thanks for the continued work on this PR!

Things are looking pretty good with only the final test of the additional code in manager/runner.go. I'll be happy to help out if needed, just ask any questions here.

@jrwren
Copy link
Contributor Author

jrwren commented Mar 9, 2022

I added a test to cover the changes in runner.go.

Thank goodness for CircleCI, because I still can't run these on my mac. I don't get the output of failed tests, even with go test -v, but this must be something my fault on this end.

@eikenb
Copy link
Contributor

eikenb commented Mar 15, 2022

Thanks for the test @jrwren, that seems to cover the tests well enough.

I have one "last" thought that occurred to me while looking it over to refresh my memory. That is that it is possible that the commands (template and exec) could be run on that first pass of Run if there are no dependencies. That is the first call to Run usually just registers all the dependencies and starts the async process of looking them up and would work fine for this but if you have no dependencies then it would just be finished and run the commands.

Though IMO this is a small issue (why use consul-templates if you have no dependencies to look up) and not worthy of changing anything. I mainly wanted to mention it here for historical reasons in case it comes up later.

TLDR; LGTM

Thanks for your work on this!

@eikenb eikenb merged commit 0220c17 into hashicorp:master Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants