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 helpers.shema.Provisoner base type for provisioners like there's one for providers #10422

Closed
wants to merge 9 commits into from
Closed

Conversation

VladRassokhin
Copy link
Contributor

Migrate provisioners to providers-like hierarchy when all of then extends schema.Provisioner.
Built-in plugins were migrated.
Also added support for schema validation for built-in provisioners: file, local-exec, remote-exec.

AFAIU external plugins would still compile but functions Apply and Validate from schema.Provisioner would not be used, so change is quite safe. Tried that by reverting cahnges in 'chef' provisioner in separate commit

@mitchellh
Copy link
Contributor

Thanks this is a great start, but I'm not ready to merge as-is. I believe we should have the provisioners use the ResourceData structure similar to Providers, for easy config access.

I might be willing to make these changes since we need to support the Stop API in core for provisioners (just doesn't yet) to make them interruptable... and I wanted to make that easy by having a helper/schema-type thing.

Known problems:
* had to introduce `ResourceData.GetRawConfig()` since chef provisioner parses configuration manually;
* In `remote-exec` provisioner `inline` can no longer accept 'string' value, only list of strings allowed. - due to migration to schema. There's a workaround like it;s done in `chef` provisioner, but I'd like to keep code as is.
@VladRassokhin
Copy link
Contributor Author

VladRassokhin commented Dec 1, 2016

Hi @mitchellh
Thanks for fast feedback.
I've updated PR with your suggestion.

As mentioned in commit message:
Known problems:

  • had to introduce ResourceData.GetRawConfig() since chef provisioner parses configuration manually;
  • In remote-exec provisioner inline can no longer accept 'string' value, only list of strings allowed. - due to migration to schema. There's a workaround like it;s done in chef provisioner, but I'd like to keep code as is.

So test failure is expected,

@VladRassokhin
Copy link
Contributor Author

If you think it's worth to spend time, I'd try to convert chef provisioner to use Schema and get rid of ResourceData.GetRawConfig()

* `chef.Provisioner` structure left intact but now it's decoded from `schema.ResourceData` instead of `terraform.ResourceConfig` suing simple copy-paste-bsed solution;
 * Added simple schema without any validation yet;
 * `ResourceData.GetRawConfig` removed since it's not longer needed.
@VladRassokhin
Copy link
Contributor Author

Hi @mitchellh, could you please take a look and provide some feedback? Maybe that PR could be merged before 0.8?

@mitchellh
Copy link
Contributor

Hey @VladRassokhin, thanks for this! I'm sorry to pre-empt your work but in introducing a Stop API for provisioners I ended up doing this in #10934. Thanks for your work and I'm sorry I didn't get this merged prior. :(

@mitchellh mitchellh closed this Dec 28, 2016
@VladRassokhin
Copy link
Contributor Author

Ok, fine. At least provisioners now has schema so I could rebase #3508 to use it and it would simplify generating metadata for IntelliJ plugin.

@VladRassokhin VladRassokhin deleted the f-provisioners-api branch December 28, 2016 10:33
@VladRassokhin VladRassokhin restored the f-provisioners-api branch February 17, 2017 01:45
@ghost
Copy link

ghost commented Apr 16, 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 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants