Add "file" type for inventory

[fiftin]

No description provided.

[strangeman]

Is this necessary? In my opinion, solutions, that simply duplicates existing ansible-playbook options is not good, they make a huge mess with Extra CLI options.
We always can pass an inventory file via Extra CLI options, no need to make the separate field on a form. This is the way to hell because ansible-playbook has around 30 various flags and we cannot add them all, it will make form completely unusable.

[fiftin]

I think this is most important flag. I use it each time when I use ansible-playbook. Adding it as Extra CLI options absolutely unusable. I sure this option will be popular.

[twhiston]

I think the current 'accepted' way of doing this, a blank inventory and a cli command, seems super hacky and unintuitive. What I like about adding a file field to this is that essentially then you can upload ec2.py (or whatever) in your repo and pass it as the inventory flag, so this effectively makes it possible to cover all dynamic inventory use cases, without any major changes (or am i wrong about this?)
I would suggest though that we remove all the other option in the list as they are disabled anyway and this removes the need for them

[strangeman]

Okay, your arguments are persuasive. :)

[twhiston]

@fiftin can you remove the other inventory type options in the be and fe so that we are left with just static and file?

[fiftin]

Done

[fiftin]

I found security issue in my solution. User can enter something like this ../../file or C:\file. I want to add inventory path validation but not sure how to do it to support Windows.

What do you this about this:

func isValidInventoryPath(path string) bool {
	if currentPath, err := filepath.Abs("./"); err != nil {
		return false
	} else if absPath, err := filepath.Abs(path); err != nil {
		return false
	} else if relPath, err := filepath.Rel(currentPath, absPath); err != nil {
		return false
	} else {
		return !strings.HasPrefix(relPath, "..")
	}
}

[twhiston]

maybe you could use os.Getwd() here instead
https://golang.org/src/os/getwd.go

[twhiston]

also you might need to look at using https://golang.org/pkg/os/#pkg-constants for the filepath separator to make things cross platform compatible if getwd is not suitable

[fiftin]

Added gopkg.in/check.v1

[twhiston]

I would prefer if you did not use that test package, it is not really necessary to add it to do the checks you are performing here

[fiftin]

What test do you mean?

[fiftin]

Why not use unit test framework?

[twhiston]

I dont honestly think that you need or want unit test frameworks in go in 99% of cases.

In your test case you can just say

if !projects.IsValidInventoryPath("inventories/test"){
  t.Fatal(" path with attibute x should be valid")
}

That is all you need!

Something so simple should not be replaced by a third party dependency

[fiftin]

Where I should place this test?

[twhiston]

So i would replace your current tests with a simple test file such as the util_test.go implementation. It should be in the api/projects dir and should be called inventory_test.go
See https://golang.org/pkg/testing/ for the basics around where tests should be and how they should be named

It would basically look like this:

package projects

import (
	"testing"
)

func TestIsValidInventoryPath(t *testing.T) {

         if !projects.IsValidInventoryPath("inventories/test"){
             t.Fatal(" a path below the cwd should be valid")
         }

...

and so on inside the function for all your test cases

[twhiston]

I also use goland to run my unit tests (and even better code coverage :D), but you dont need anything special for this or any external libraries. IMO we should be implementing tests in as native a manner as possible, and that means doing it in the way the language specification describes, putting them in the same package etc...... as per the above link to the testing package.

[fiftin]

Done

[twhiston]

I'm not sure that this will ever get run, as it's impossible to test in windows on CircleCI 😬
But it's good to have as a test anyway!

[twhiston]

Just tested this and it is working great with files for inventories (including python scripts) so we now have dynamic inventory support thanks to your efforts. Great work!