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

Use helpers.shema.Provisoner in Chef provisioner V2 #14681

Merged
merged 3 commits into from
May 30, 2017
Merged

Use helpers.shema.Provisoner in Chef provisioner V2 #14681

merged 3 commits into from
May 30, 2017

Conversation

svanharmelen
Copy link
Contributor

Done by @VladRassokhin:

  1. Migrate chef provisioner to schema.Provisioner:
  • chef.Provisioner structure was renamed to ProvisionerSand now it's decoded from schema.ResourceData instead of terraform.ResourceConfig using simple copy-paste-based solution;
  • Added simple schema without any validation yet.
  1. Support ValidateFunc validate function : implemented in file and chef provisioners.

Done by @svanharmelen:

  1. The tests did pass, but that was because they only tested part of the changes. By using the schema.TestResourceDataRaw function the schema and config are better tested and so they pointed out a problem with the schema of the Chef provisioner.

  2. So fixed the Elem fields that did not have a *schema.Schema but a schema.Schema and in an Elem schema only the Type field may (and must) be set. Any other fields like Optional are not allowed here.

  3. Next to fixing that problem I also did a little refactoring and cleaning up. Mainly making the ProvisionerS private (provisioner) and removing the deprecated fields.

VladRassokhin and others added 3 commits May 19, 2017 20:43
1. Migrate `chef` provisioner to `schema.Provisioner`:

 * `chef.Provisioner` structure was renamed to `ProvisionerS`and  now it's decoded from `schema.ResourceData` instead of `terraform.ResourceConfig` using simple copy-paste-based solution;
 * Added simple schema without any validation yet.

 2. Support `ValidateFunc` validate function : implemented in `file` and `chef` provisioners.
The tests did pass, but that was because they only tested part of the changes. By using the `schema.TestResourceDataRaw` function the schema and config are better tested and so they pointed out a problem with the schema of the Chef provisioner.

The `Elem` fields did not have a `*schema.Schema` but a `schema.Schema` and in an `Elem` schema only the `Type` field may (and must) be set. Any other fields like `Optional` are not allowed here.

Next to fixing that problem I also did a little refactoring and cleaning up. Mainly making the `ProvisionerS` private (`provisioner`) and removing the deprecated fields.
Copy link
Contributor

@grubernaut grubernaut left a comment

Choose a reason for hiding this comment

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

LGTM, may warrant another look-over from @mitchellh though.

@svanharmelen
Copy link
Contributor Author

Thx, @grubernaut!

@mitchellh I guess @grubernaut is referring to you for the change made in helper/schema/provisioner.go where a ValidateFunc is added to the Provisioner struct.

I found it a very helpful addition that we should/could maybe also add to the Provider struct? But it's good to hear your take on that one.

The rest of the changes are mainly updates and some refactoring of existing stuff, so I don't think there's anything exciting in the other parts...

Thx!

@svanharmelen
Copy link
Contributor Author

@grubernaut @mitchellh @stack72 can we move this one forward? Even though this PR isn't open for that long, the PR that is included in this PR was open since February. So it would be nice to get this one merged before the next release... Thx!

@grubernaut grubernaut merged commit 7894478 into hashicorp:master May 30, 2017
@svanharmelen svanharmelen deleted the f-review branch May 30, 2017 18:28
@onorua
Copy link

onorua commented Jun 12, 2017

This actually broken file provisioning, since release 0.9.7 till 0.9.8 (up until now):
#15177

if _, ok := d.GetOk("source"); ok == true {
numSrc++
}
if _, ok := d.GetOk("content"); ok == true {
Copy link

Choose a reason for hiding this comment

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

it seems GetOk fail to return ok in the following part:
https://github.com/hashicorp/terraform/blob/master/helper/schema/resource_data.go#L91

The struct is:

r: {${data.template_file.config.rendered} <nil> true true 0xc4203c12c0}

Which means r.Exists and r.Computed but expecting r.Exists && !r.Computed
I was able to make a workaround with following code:

func validateFn(d *schema.ResourceData) (ws []string, es []error) {
	numSrc := 0
	content := d.Get("content").(string)
	source := d.Get("source").(string)

	if content != "" {
		numSrc++
	}
	if source != "" {
		numSrc++
	}

	if numSrc != 1 {
		es = append(es, fmt.Errorf("Must provide one of 'content' or 'source'"))
	}
	log.Printf("Numsrc is: %v", numSrc)
	return
}

@VladRassokhin
Copy link
Contributor

@onorua Sorry for causing the issue, seems functions not working the way I expected. And seems tests are not coveting such scenario. It would be nice if you could also add test case in your pull request which fixes problem.

@onorua
Copy link

onorua commented Jun 12, 2017

@VladRassokhin yeah, I was amused with behavior of GetOk as well, will work on test cases.

@ghost
Copy link

ghost commented Apr 11, 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 11, 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.

4 participants