Adding support for nomad variables #325
Conversation
Work based on feedback of the original PR for variables. * Added more descriptions and updated a few existing ones * Added validation for the path attribute * Reworked a bit of the implementations, made them more closely resemble other implementations (such as acl_auth_method) for consistency * First pass adding a test
|
|
|
@lgfa29 sorry for the ping, wasn't sure who else to poke, would you be able to help move this PR forward? |
* Adjusting error formats to conform to standards * Fixed the resource read so it works for the data_variable * Removed redundant return
|
I am unsure how best to handle the path changing. That requires deleting the old resource and creating a new one. |
|
Hey @regner, I just came across this PR and realize it hasn't gotten a reply yet! Sorry! I'll make sure we have somebody take a look next week. |
There was a problem hiding this comment.
Looking good @regner, thanks for picking-up the work from @iluminae.
I left a few more notes here, and would you be able to write some documentation as well? They live inside the website forlder. Unfortunately the local doc preview flow is not working anymore, but you can use this website for now: https://registry.terraform.io/tools/doc-preview
Let me know if you have any questions 🙂
* Fixed some typos * Added test for path name changing * Now return multiple errors at once instead of only one error when validating variable path
|
Thank you very much for the comments. I have updated everything but the docs, which I will work on now. |
|
@lgfa29 I have now added the docs. I think it's ready for another review when you have time. Thank you very much for your time and help with this. |
|
Just realized the tests need a min version check for variables. |
|
Is there a way to run a specific test or do I need to always run all the tests? |
There was a problem hiding this comment.
Thank you for the fixes @regner!
I noticed some other things I didn't notice in the first review (like namespace needing to be ForceNew as well) and others mistake I made with the error handling where some errors ended up being ignore. I pushed 2a9a0c0 to fix those and some other minor things. Hopefully I didn't break anything else 😅
For this I usually run the |
|
Thank you very much for your help and time with this PR. I learned a bunch. |
This PR is based on #304. The original author, iluminae, of that PR is unable to continue their work.
Please note that I have not done Terraform provider work before, and very little Go work, so if some choices or questions don't make sense, that's probably why.
I have tried to implement the feedback that @lgfa29 had on the original PR. I did however only add a single test, I wanted to make sure I was on the write path before continuing.