(WIP) Added Validation to Packer Binary Wrapper #27

Hrily · 2017-10-08T05:40:42Z

PR for #21

I was unsure if I'm going in right direction.

I have just implemented validation part, please have a look and tell if this is good.

PS: Are we taking care of -var in Template itself???

coveralls · 2017-10-08T05:42:14Z

Coverage decreased (-5.3%) to 75.888% when pulling fb671ed on Hrily:master into 2e61b71 on mayn:master.

mayn · 2017-10-09T03:26:08Z

@Hrily this is a great start!
yes variables are in the template (https://github.com/mayn/packerlicious/blob/master/src/packerlicious/template.py#L42) . here is them in action from the example https://github.com/mayn/packerlicious/blob/master/examples/getting_started_example.py#L33

I'll leave you some comments for this PR.
I'll also make some additional tickets for initial non-required changes. Your choice if you want to do them all in 1 PR or make some follow-up PRs.

thanks for your effort!
p.s. will need need to add unit tests in the end😉

Hrily · 2017-10-09T04:27:07Z

@mayn
That's good to hear... :)

yes variables are in the template

Okay, then I'll remove them from packer.py

p.s. will need need to add unit tests in the end:wink:

Sure 👍

mayn

@Hrily here is some feedback.
let me know your thoughts!

mayn · 2017-10-09T03:29:43Z

src/packerlicious/packer.py

+            packer_path += "/"
+        self.packer_path = packer_path
+        self.template = template
+        self._vars = _vars


vars are already an attribute of template and should be accesible via template. variables

Will do this 👍

mayn · 2017-10-09T03:42:56Z

src/packerlicious/packer.py

+        if len(packer_path) > 0 and packer_path[-1] != '/':
+            packer_path += "/"
+        self.packer_path = packer_path
+        self.template = template


i see template as just a method parameter vs an instance variable. and Packer class as stateless
so it is easy to use:

packer.validate(my_template) packer.validate(my_template2)

or whatever without having to re-initial a packer object.

what do you think?

Okay 👍

So each method will require template as parameter.
Also Should the packer binary path be the parameter for each method?
I did it because, I did not make assumption whether user has updated path variable w.r.t. packer.

So each method will require template as parameter.

correct

Also Should the packer binary path be the parameter for each method?

I understand. I think it is fine the way it is as a class variable. A future enhancement could be to look at os.env variable or something if it is useful

For now I think this is a fair expectation that the user know where packer is on their system and provide the location if necessary.

The check_available method should catch if packer isn't found in the location

Okay

So each method will require template as parameter.

I think we won't require Packer class. All methods will be part of packer.

But in this case, packer_path should be a parameter for each method, since we don't have class.

What do you think?
Correct me if I'm wrong.

try it out, can always change it back. so long as you aren't making packer_path required for the user to provide i think it should be fine.

mayn · 2017-10-09T04:13:10Z

src/packerlicious/packer.py

+        for key in self._vars:
+            validate_command += "-var '" + key + "=" + self._vars[key] + "' "
+        template_file, template_filename = tempfile.mkstemp()
+        os.write(template_file, self.template.to_json())


ideally this would pipe directly to packer without creating a tmp file.
if you keep the tmp file writing, please add error handling

if you don't change it on this pr, please create an issue about this so the idea doesn't get lost.

I did not find a way to pipe it directly without creating file.
If you have a way, plz tell me so I can update accordingly.

may be able to do it via stdin of the popen.communicate. see hashicorp/packer#2914 for an example. looks like there is a trailing - they pass to the cmd.

Also more info here hashicorp/packer#4132

Yes, did it 👍
Thanks :)

mayn · 2017-10-09T04:25:49Z

src/packerlicious/packer.py

+        proc = subprocess.Popen(shlex.split(validate_command), stdout=subprocess.PIPE)
+        out = proc.communicate()[0]
+        os.remove(template_filename)
+        return proc.returncode, out


would be nice to handle return by converting to a python object we can interact with.

please create an issue so we don't loose this thought ( don't expect as part of initial submission)

Will do this 👍

mayn · 2017-10-09T04:28:01Z

src/packerlicious/packer.py

+        os.write(template_file, self.template.to_json())
+        os.close(template_file)
+        validate_command += template_filename
+        proc = subprocess.Popen(shlex.split(validate_command), stdout=subprocess.PIPE)


should capture and return stderr also

Will do this 👍

coveralls · 2017-10-09T05:15:42Z

Coverage decreased (-9.4%) to 71.795% when pulling dabe2df on Hrily:master into 2e61b71 on mayn:master.

coveralls · 2017-10-09T05:38:05Z

Coverage decreased (-9.7%) to 71.479% when pulling a9d66c0 on Hrily:master into 2e61b71 on mayn:master.

coveralls · 2017-10-09T06:04:44Z

Coverage decreased (-7.8%) to 73.415% when pulling 9b14f14 on Hrily:master into 2e61b71 on mayn:master.

coveralls · 2017-10-10T07:51:54Z

Coverage decreased (-9.6%) to 71.604% when pulling d2a5b23 on Hrily:master into 2e61b71 on mayn:master.

Hrily · 2017-10-10T07:52:37Z

I did the mentioned changes, please review.

Sorry for late push, was travelling yesterday.

Also, Can you tell me how would I pass this coverage test?

mayn · 2017-10-10T12:06:03Z

thanks @Hrily!
re: test coverage. you click over into the coveralls coverage report for your pr.
Basically you need to add the appropriate pytests for the code being added. Currently there isn't any so this is causing the project coverage to decrease and build to fail. see packer.py coverage for your last commit (https://coveralls.io/builds/13644387/source?filename=src%2Fpackerlicious%2Fpacker.py)

Will review over next day...

Hrily · 2017-10-13T08:00:17Z

@mayn

Could you give me some test cases for packer.py

I think I have written some wrong test case... 🙈

mayn · 2017-10-14T15:09:33Z

@Hrily the tests are failing because packer binary is not currently present on the ci systems (#63) for integration testing.

From a unit test perspective you would have to mock/patch the calls to the OS to emulate the interaction to packer binary.

Hrily · 2017-10-22T13:44:27Z

Hie @mayn

Sorry for a real late reply.

The build test on travis is failing with this output

amazon-ebs output will be in this color.

==> amazon-ebs: Prevalidating AMI Name...
==> amazon-ebs: Error querying AMI: NoCredentialProviders: no valid providers in chain
==> amazon-ebs: caused by: EnvAccessKeyNotFound: failed to find credentials in the environment.
==> amazon-ebs: SharedCredsLoad: failed to load profile, .
==> amazon-ebs: EC2RoleRequestError: no EC2 instance role found
==> amazon-ebs: caused by: RequestError: send request failed
==> amazon-ebs: caused by: Get http://169.254.169.254/latest/meta-data/iam/security-credentials: dial tcp 169.254.169.254:80: getsockopt: no route to host
Build 'amazon-ebs' errored: Error querying AMI: NoCredentialProviders: no valid providers in chain
caused by: EnvAccessKeyNotFound: failed to find credentials in the environment.
SharedCredsLoad: failed to load profile, .
EC2RoleRequestError: no EC2 instance role found
caused by: RequestError: send request failed
caused by: Get http://169.254.169.254/latest/meta-data/iam/security-credentials: dial tcp 169.254.169.254:80: getsockopt: no route to host

==> Some builds didn't complete successfully and had errors:
--> amazon-ebs: Error querying AMI: NoCredentialProviders: no valid providers in chain
caused by: EnvAccessKeyNotFound: failed to find credentials in the environment.
SharedCredsLoad: failed to load profile, .
EC2RoleRequestError: no EC2 instance role found
caused by: RequestError: send request failed
caused by: Get http://169.254.169.254/latest/meta-data/iam/security-credentials: dial tcp 169.254.169.254:80: getsockopt: no route to host

==> Builds finished but no artifacts were created.

Return code: 1
Error: None

Can I get help regarding this???

I'm using the same Templates used by example.

Hrily · 2017-11-11T04:28:04Z

@mayn
Any updates for my last comment?

mayn · 2017-12-01T03:40:36Z

@Hrily apologies for the delay, I think you are getting the error because the example template doesn't actually have ec2 credentials in it.
You'd have to provide valid credentials or create a simple packerlicious example to use that doesn't add aws as a layer

mayn · 2018-03-25T23:49:11Z

Build an alternate implementation as standalone repo https://github.com/mayn/packer.py

Added Validation to Packer Binary Wrapper

fb671ed

mayn reviewed Oct 9, 2017

View reviewed changes

Added Build, Inspect, Push to Packer Binary Wrapper

dabe2df

Resolved some of the comments on PR mayn#27

a9d66c0

Template now piped directly to packer command

9b14f14

mayn changed the title ~~Added Validation to Packer Binary Wrapper~~ (WIP) Added Validation to Packer Binary Wrapper Oct 10, 2017

Added template as parameter for all methods

d2a5b23

Added Unit tests for packer.py

dd5592b

mayn mentioned this pull request Oct 13, 2017

install latest packer version as part of the setup for travis / appveyor build environment #63

Closed

mayn closed this Oct 20, 2017

mayn reopened this Oct 20, 2017

mayn added the wontfix label Mar 25, 2018

mayn closed this Mar 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) Added Validation to Packer Binary Wrapper #27

(WIP) Added Validation to Packer Binary Wrapper #27

Hrily commented Oct 8, 2017

coveralls commented Oct 8, 2017

mayn commented Oct 9, 2017 •

edited

Loading

Hrily commented Oct 9, 2017

mayn left a comment

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

mayn Oct 9, 2017

Hrily Oct 9, 2017

coveralls commented Oct 9, 2017

coveralls commented Oct 9, 2017

coveralls commented Oct 9, 2017

coveralls commented Oct 10, 2017

Hrily commented Oct 10, 2017

mayn commented Oct 10, 2017 •

edited

Loading

Hrily commented Oct 13, 2017

mayn commented Oct 14, 2017

Hrily commented Oct 22, 2017

Hrily commented Nov 11, 2017

mayn commented Dec 1, 2017 •

edited

Loading

mayn commented Mar 25, 2018

(WIP) Added Validation to Packer Binary Wrapper #27

(WIP) Added Validation to Packer Binary Wrapper #27

Conversation

Hrily commented Oct 8, 2017

coveralls commented Oct 8, 2017

mayn commented Oct 9, 2017 • edited Loading

Hrily commented Oct 9, 2017

mayn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 9, 2017

coveralls commented Oct 9, 2017

coveralls commented Oct 9, 2017

coveralls commented Oct 10, 2017

Hrily commented Oct 10, 2017

mayn commented Oct 10, 2017 • edited Loading

Hrily commented Oct 13, 2017

mayn commented Oct 14, 2017

Hrily commented Oct 22, 2017

Hrily commented Nov 11, 2017

mayn commented Dec 1, 2017 • edited Loading

mayn commented Mar 25, 2018

mayn commented Oct 9, 2017 •

edited

Loading

mayn commented Oct 10, 2017 •

edited

Loading

mayn commented Dec 1, 2017 •

edited

Loading