(WIP)(CLOUD-184, CLOUD-205) Make the region param and ENV var interchangable #117
Conversation
garethr
changed the title
Iristyle
changed the title
| @@ -98,6 +98,12 @@ def self.tags_for(item) | |||
| tags | |||
| end | |||
|
|
|||
| def target_region | |||
| target = resource ? resource[:region] || region : region | |||
There was a problem hiding this comment.
Where does region come from? Should this method have a parameter list (region = default_region)?
There was a problem hiding this comment.
region in this context is the value of the region property. This basically says:
- If you specify a region (ie. on the command line like
puppet resource ec2_securitygroup bob region=blahor in the DSL, use that - If that isn't present, fall back to the value of region for the resource
|
Agreed. I'm unsure about this too. I'm tempted to say if the env variable ever doesn't match the region value of a type we fail in the type validation. That blocks the entire run. Thoughts? I think we should do that whether we merge this PR or not. |
|
Yes @garethr I was talking with @cmurphy and this seems like a tough nut to crack. At first I suggested that we could use apply a rule that So your suggestion is to blow up the run when the resource defined in the manifest has a region that doesn't match the env var? I think that's probably a good thing to do... |
|
@Iristyle yup. I like the filtering ability. It's a great performance optimisation, and it would allow for scaling out by region if you had lots of ec2 resources. But that power comes with complexity that I don't like. If we stop the run at compile time if any resource has a region that doesn't match the ENV then I think it's much safer. It means we never hit issues like the one you describe. We also catch issues we're you forget you have the ENV set. |
This allows for creating and deleteing resources from puppet resource by passing the environment variable (which is already used for scoping). Prior to this change you would have to pass the information twice. If you do pass both we maintain the same approach as previous, giving the param a higher presendence. This also allows for not specifying the region in the DSL, and instead using the AWS_REGION variable. This matches the standard behaviour of other AWS tools, and means in the case of not providing anything the correct error bubbles through.
garethr
force-pushed
the
delete-using-env-var
branch
from
8a86e1f
to
40ae3b2
Compare
If using the AWS_REGION environment variable and a manifest where the region property of one or more resources doesn't match you can see unspecified behaviour. See puppetlabs-toy-chest#117 for discussions. This PR extracted from the above simply stops that from happening. If the env is present and doesn't match All of the region properties then we fail. This maintains the utility of the env var as an optimisation but stops us shooting ourselves in the foot.
|
I've extracted the region type and made a separate PR for just the blocking behaviour #132 |
garethr
changed the title
|
I'm going to close this. It has a bunch of unanswered questions. It's hopefully useful as and when the core issue gets some time. |
If using the AWS_REGION environment variable and a manifest where the region property of one or more resources doesn't match you can see unspecified behaviour. See puppetlabs-toy-chest#117 for discussions. This PR extracted from the above simply stops that from happening. If the env is present and doesn't match All of the region properties then we fail. This maintains the utility of the env var as an optimisation but stops us shooting ourselves in the foot.
(Note that I've put this up just applying to security groups so as to get feedback on whether we should do some/all/none this as much as the implementation)
This allows for creating and deleting resources from puppet resource by
passing the environment variable (which is already used for scoping).
Prior to this change you would have to pass the information twice. If
you do pass both we maintain the same approach as previous, giving the
param a higher precedence.
This also allows for not specifying the region in the DSL, and instead
using the AWS_REGION variable. This matches the standard behaviour of
other AWS tools, and means in the case of not providing anything the
correct error bubbles through.