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

Added validate command #3783

Merged
merged 1 commit into from
Feb 8, 2016
Merged

Added validate command #3783

merged 1 commit into from
Feb 8, 2016

Conversation

sorenmat
Copy link
Contributor

@sorenmat sorenmat commented Nov 6, 2015

Added verify command to validate .tf files.

This can be usefull if you want to verify your terraform files on a CI server or as a webhook for
verifying commits to a repository.
It's a simpler verfication than running a terraform plan.
It will verify:

  • Missing or undefined variables
  • HCL structure error i.e. wrongly named/missing references

@sorenmat
Copy link
Contributor Author

@radeksimko Any decision on this one ?

@radeksimko
Copy link
Member

Hey @sorenmat ,
I really like the idea. I was myself looking for something like this in our CI, I just didn't have time to review the code thoroughly.

Also it would help the reviewer & users if you attached docs describing the purpose of that command - i.e. what are the typical errors/mistakes that will be detected and won't be detected.

e.g. circular dependencies, offline validation (ValidateFunc in schema), invalid references, invalid HCL syntax, ... ?


It's not quite solving #3190 yet, is it? (ain't saying it should)

@sorenmat
Copy link
Contributor Author

I think it's pretty close to solving #3190 from how I read it. Atleast that is how we use it.
We run it on our CI servers to validate the .tf files for syntax errors.
So it kinda works like plan in offline mode.

@josephholsten
Copy link
Contributor

@sorenmat this seems like a no-brainer. Got time to update this onto master? If not, I'll gladly do it.

@phinze, @mitchellh this could use your 👀. Has tests, needs doc & rebase.

@josephholsten
Copy link
Contributor

lol, the only merge conflict was in .gitignore. rebased up in #5004

@sorenmat
Copy link
Contributor Author

sorenmat commented Feb 5, 2016

@josephholsten I rebased an added some documentation to website/source/docs/commands
If someone could proof read it, that would be awesome.

@josephholsten
Copy link
Contributor

looks great!

@phinze this is fully ready for your 👀, has tests & doc.

@radeksimko
Copy link
Member

Functionally this looks great.

There's a few things I'd like to discuss though:

  • the term verify is often used in the context of formal (e.g. by certification authority) verification which ends with getting a certificate. I'd say that validate is more precise term for what we're doing here. After all most of the underlying method names are also using this term. Would you mind renaming the command?
  • The test case you included is great, however would you mind also adding a few more which would cover other failures? It doesn't have to be covering all the cases, although it would be awesome 🏆 , but 2-3 more would be nice. See my list below. ⬇️
  • I think we should mention some examples in the documentation (i.e. what is considered a failure) ⬇️
  • Can you link the new docs page from website/source/layouts/docs.erb so people can actually get to it from the menu?
 * invalid [HCL](https://github.com/hashicorp/hcl) syntax (e.g. missing trailing quote or equal sign)
 * invalid HCL references (e.g. variable name or attribute which doesn't exist)
 * same `provider` declared multiple times
 * same `module` declared multiple times
 * same `resource` declared multiple times
 * same `output` declared multiple times
 * invalid `module` name
 * interpolation used in places where it's unsupported


The `terraform verify` command is used to validate the format and structure of the terraform files.
Terraform performas an analysis of the terraform files, and will display information about missing
variables and errors in the structure of the files.
Copy link
Member

Choose a reason for hiding this comment

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

Can we reword this a little bit to not mention format and structure? What we're really checking is syntax mostly.

Formatting will be checked by fmt per #4955 eventually.

I would in fact even explicitly mentioned in a separate paragraph that this command does not check the format of your code. We may expand that paragraph later on when we merge fmt as linked above.


It's pretty much like go vet VS gofmt whereas go vet is what we do.

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 7, 2016
@radeksimko
Copy link
Member

Besides the things mentioned, this looks really good, I'm looking forward to see it in master! 😃
Thanks @sorenmat

@sorenmat sorenmat force-pushed the verify branch 3 times, most recently from a0991fc to 8de6b0b Compare February 8, 2016 11:07
@sorenmat
Copy link
Contributor Author

sorenmat commented Feb 8, 2016

@radeksimko glad to be able to help, and thanks for an awesome product !

I've renamed the command to validate and updated the documentation as requested.
I've also added a few more test to cover the checks defined in config.go.
Please note: There is no check in config.Validate that checks for multiple output's with the same name.
Is this an error / missing feature ?

If it is I will be glad to do another PR with that fix :)

@radeksimko radeksimko changed the title Added verify command Added validate command Feb 8, 2016
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 8, 2016
@radeksimko
Copy link
Member

There is no check in config.Validate that checks for multiple output's with the same name.

Good catch! I just opened a new issue for this: #5052
If you want to send another PR to fix that, that's fine, I believe this is out of scope for this PR though.

I think this is good to go! 🚢

radeksimko added a commit that referenced this pull request Feb 8, 2016
@radeksimko radeksimko merged commit e968bfd into hashicorp:master Feb 8, 2016
@ghost
Copy link

ghost commented Apr 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants