enhance put.inputs detect to ingore prefixed . and ..

[evanchaoli]

Bug Fix | Feature | Documentation

closes #6704 .

Changes proposed by this PR:

Ignore prefixed . and .. of put param paths.

Notes to reviewer:

Release Note

Enhanced put.inputs:

• input: detect now can handle paths prefixed by . and ...

Contributor Checklist

• Followed Code of conduct, Contributing Guide & avoided Anti-patterns
• Signed all commits
• Added tests (Unit and/or Integration)
• Updated Documentation
• Added release note (Optional)

Reviewer Checklist

• Code reviewed
• Tests reviewed
• Documentation reviewed
• Release notes reviewed
• PR acceptance performed
• New config flags added? Ensure that they are added to the
  BOSH and
  Helm packaging; otherwise, ignored for
  the integration
  tests
  (for example, if they are Garden configs that are not displayed in the
  --help text).

[taylorsilva]

Could you add a test for this in put_step_test.go

concourse/atc/exec/put_step_test.go

Lines 343 to 348 in 523345c

	Context("when the inputs are detected", func() {
	BeforeEach(func() {
	putPlan.Inputs = &atc.InputsConfig{
	Detect: true,
	}
	})

One question that I still have after reading the issue; why would you put ../ as the path? Is this a quirk of the resource the put step represents in this instance? I'm worried that allowing users to specify ../ as the start of their path will confuse them when they try to think about how Concourse makes the inputs available to the resource.

In the issue you gave this as the example

- put: some-resource
  params:
    file: ./build/some-file
    vars: ../vars/some-var-file

So in the resource container, it's expected that both build and vars will be available at the same point in the filesystem.

/tmp/inputs
├── build
└── vars

Whereas the ../ syntax makes it seem like the artifact structure will look like:

/tmp
├── vars
└── inputs
    └── build

Are you able to explain why ../?

[evanchaoli]

Could you add a test for this in put_step_test.go

@taylorsilva Sure. I should have left a comment to say that, I planned to add new tests, just haven't got bandwidth to work on that yet. I'll add tests soon.

[evanchaoli]

Are you able to explain why ../?

@taylorsilva I can list all 3 issues reported by our users after they adding inputs: detect. There are all from our users' real production pipelines.

1. The first one used docker-image resource:

- put: img
  params:
     build: ./outputs
     dockerfile: ./outputs/Dockerfile

I asked the user to fix the problem by removing ./. But we are not able to review all pipeline source code before they are submitted, and technically ./build/ equals to build/. So I think detect should support form of ./build.

2. The second one use the built-in cf resource type:

- put: app
  params:
    path: auth
    vars_files:
     - ../vars/vars.yml
     # other params

I didn't check source code of cf-resource, but I guess it changes current worker directory, so that var file has to use ../. The user fixed the problem by changing detect to inputs: [auth, vars].

3. The third error is caused by implicit path with docker-image resource:

- put: docker-image
  params:
    build: "."
    dockerfile: "source/Dockerfile"
    tag_file: version/version
    # other params

The user fixed the error by changing detect to inputs: [build, source, version]. Here we can see that, build doesn't present in params, it should be a path that is used in Dockerfile. From this case, we can see that, it might be impossible to detect all dependent artifacts from params.

[taylorsilva]

The second one use the built-in cf resource type:

I'm not at all familiar with the cf resource either. Not sure why they'd have to do that. I'm fine with keeping ../ in the detect logic since it appears it will help detect work better. Can't think of a case where it may break things for anyone or cause extra volumes to be streamed to the put step.

[taylorsilva]

Thanks Evan! LGTM