concourse / concourse

Concourse is a container-based continuous thing-doer written in Go.
https://concourse-ci.org
Apache License 2.0
7.37k stars 846 forks source link

resource names in across steps are not interpolated #7560

Open geofffranks opened 2 years ago

geofffranks commented 2 years ago

Summary

When using static variables in an across step, I'm unable to do a get or put using variable interpolation to determine the steps

Steps to reproduce

Try to fly this pipeline:

groups:
- name: main
  jobs:
  - unit-tests

jobs:
- name: unit-tests
  plan:
  - across:
    - var: release
      values: [ cf-networking, silk ]
    do:
    - get: repo-to-test
      resource: ((.:release))-dev

resources:
- name: cf-networking-dev
  type: git
  source:
    uri: git@github.com:cloudfoundry/cf-networking-release
    branch: develop
- name: silk-dev
  type: git
  source:
    uri: git@github.com:cloudfoundry/silk-release
    branch: develop

Expected results

It should create a job that successfully pulls both resources

Actual results

It fails with an error like this:

invalid jobs:
        jobs.unit-tests.plan.do[0].across.do[0].get(((.:release))-dev): unknown resource '((.:release))-dev'

Additional context

I don't anticipate needing to do something like this across a load_var-based set of values for the across, just static.

Triaging info

taylorsilva commented 2 years ago

The error for this is coming from the pipeline validation code: https://github.com/concourse/concourse/blob/master/atc/configvalidate/validate.go

I'm feeling an awkwardness between the across step and how resources work in a pipeline here.

I kinda get what you're trying to do here, with the across step going over multiple repo's and running tests in each one. I feel like this is being poorly serviced by the git resource though.

I think this could be fixed in one of two ways.

1 - currently the pipeline validation logic expects every resource in the pipeline to be used and checks that every resource called in get and put steps has been declared. We could relax/remove this check. I could then see people opening up bugs saying we didn't warn them about unused resources in their pipeline. This check is a pretty hand safety check for users writing pipelines. I don't think I'm a fan of going down this path

2 - From your example it looks like you want to run tests across a set of repos. Without changing anything in Concourse or the resource type being used, you could something like this:

jobs:
- name: unit-tests
  plan:
  - get: cf-networking
    resource: cf-networking-dev
  - get: silk
    resource: silk-dev
  - across:
    - var: release
      values: [ cf-networking, silk ]
    task: run-tests
    input_mapping:
      repo: ((.:release))
    config:
      platform: linux
      image_resource:
        type: mock
        source:
          mirror_self: true
      inputs:
        - name: repo
      run:
        path: /bin/sh
        args:
          - -cx
          - |
            ls -lah repo/

resources:
- name: cf-networking-dev
  type: git
  source:
    uri: https://github.com/cloudfoundry/cf-networking-release
    branch: develop
- name: silk-dev
  type: git
  source:
    uri: https://github.com/cloudfoundry/silk-release
    branch: develop

I was curious if this would work so I actually ran it and it does!

image
geofffranks commented 2 years ago

That is how we worked around it for our pipeline, but this gets messy the more variables you add + across over.

Additionally, we're running into the same issue with set_pipeline, requiring us to have to separate jobs to set a pipeline, when we could across over both pipelines, since they use the same yaml, passing in variables

taylorsilva commented 2 years ago

I guess that part I'm trying to understand is why is this:

  - across:
    - var: release
      values: [ cf-networking, silk ]
    get: repo-to-test
    resource: ((.:release))-dev

preferred over this:

- get: cf-networking
- get: silk

Maybe I don't see it with this tiny example. You said it gets messy when it's larger? Can you share an example?

clarafu commented 2 years ago

The across step only allows you to provide values into vars that can be interpolated. There are only a few things within the pipeline config that can be interpolated such as a few of the ones listed in here https://concourse-ci.org/vars.html#dynamic-vars. One other place that allows interpolation is input_mapping (https://github.com/concourse/concourse/pull/7252 Aidan mentions it in here). I think we should definitely make it more clear and documented what parts of the pipeline can be interpolated and what cannot.

I think it would be impossible to allow for the interpolation of resource in the pipeline within the across step with the way Concourse currently works. This is because we would need to run a build of the job in order to know what resource that job is currently using, and that would result in a few complications. For example, if you have the following pipeline

jobs:
- name: unit-tests
  plan:
  - across:
    - var: release
      values: [ cf-networking, silk ]
    do:
    - get: repo-to-test
      resource: ((.:release))-dev
      trigger: true

The trigger: true means that we will want to create a new build of unit-tests when the repo-to-test input gets a new version. But we will only know if it has a new version by checking the resource, which we won't know what resource to check until we run a build of the job. As a result, we get into a circular dependency.

geofffranks commented 2 years ago

Fair enough. Would it be possible to interpolate set-pipeline’s value, or does that suffer from a similar problem?

Sent from my iPhone

On Sep 24, 2021, at 6:04 PM, Clara Fu @.***> wrote:  The across step only allows you to provide values into vars that can be interpolated. There are only a few things within the pipeline config that can be interpolated such as a few of the ones listed in here https://concourse-ci.org/vars.html#dynamic-vars. One other place that allows interpolation is input_mapping (#7252 Aidan mentions it in here). I think we should definitely make it more clear and documented what parts of the pipeline can be interpolated and what cannot.

I think it would be impossible to allow for the interpolation of resource in the pipeline within the across step with the way Concourse currently works. This is because we would need to run a build of the job in order to know what resource that job is currently using, and that would result in a few complications. For example, if you have the following pipeline

jobs:

  • name: unit-tests plan:
    • across:
    • var: release values: [ cf-networking, silk ] do:
    • get: repo-to-test resource: ((.:release))-dev trigger: true The trigger: true means that we will want to create a new build of unit-tests when the repo-to-test input gets a new version. But we will only know if it has a new version by checking the resource, which we won't know what resource to check until we run a build of the job. As a result, we get into a circular dependency.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

keevan commented 2 years ago

Disclaimer: I'm rather new to concourse.

Fair enough. Would it be possible to interpolate set-pipeline’s value, or does that suffer from a similar problem?

I've tried doing something similar myself, and it seems to work but comes up with deprecation warnings (meaning it might be removed one day..). Here's a snippet of what I was trying to do:

across:
jobs:
  name: setup-pipelines
  plan:
  - get: ci
  - file: ci/configs/pipelines.yml # contains a list of concourse-resources
    load_var: config
    - values: ((.:config.concourse-resources))
      var: resource
    set_pipeline: ((.:resource)) # <------- This line throws the deprecation warning
    file: ci/templates/pipelines/build-concourse-resource.yml
    vars:
      name: ((.:resource)) # This one is used inside the above file to reference the correct resource

I can use the ((.:resource)) variable to define the pipeline name, and it works, but setting it returns this warning in CLI which worries me:

DEPRECATION WARNING:
  - jobs.setup-pipelines.plan.do[2].across.set_pipeline(((.:resource))): '((.:resource))' is not a valid identifier: must start with a lowercase letter

I searched up relevant docs but couldn't see an open issue for it, but found something related: https://github.com/concourse/concourse/issues/3985

I found this another issue which describes the multi-branch workflow, which might work for branches, but not exactly what I wanted: https://github.com/concourse/concourse/issues/1172, since it uses instanced pipelines and would probably group the pipelines together instead of splitting them out separately as I'd prefer. (Relevant snippet below)

jobs:
- name: set-pipelines
  plan:
  - get: ci
  - set_pipeline: feature
    file: ci/pipelines/feature.yml
    instance_vars: {branch: ((branch.name))}
    across:
    - var: branch
      source: branches
      trigger: true

The trigger: true means that we will want to create a new build of unit-tests when the repo-to-test input gets a new version. But we will only know if it has a new version by checking the resource, which we won't know what resource to check until we run a build of the job. As a result, we get into a circular dependency.

From reading that code snippet, I feel like any dependent variables should be checked (where possible) and subsequently the resources checked (if the current step is at the start and not waiting for any other steps prior, or if the previous step has completed and the pipeline is now waiting for one or more triggers from the resources)

But as you said, it's only resolved during runtime of the job. The above code snippet I included was from the previous link, and seems to have a similar issue which might resolve this one as well, but alas it's using instanced pipelines.

taylorsilva commented 2 years ago

Your initial pipeline mashed the load_var and set_pipeline step together into one step. What you wanted was this:

across:
jobs:
  name: setup-pipelines
  plan:
  - get: ci
  - load_var: config
    file: ci/configs/pipelines.yml
  - across:
    - values: ((.:config.concourse-resources))
      var: resource
    set_pipeline: ((.:resource)) # <------- would still throw a deprecation warning
    file: ci/templates/pipelines/build-concourse-resource.yml
    vars:
      name: ((.:resource))

The set_pipeline line would still get that error and I'm guessing we don't interpolate that line during runtime.

keevan commented 2 years ago

Your initial pipeline mashed the load_var and set_pipeline step together into one step. What you wanted was this:

across:
jobs:
  name: setup-pipelines
  plan:
  - get: ci
  - load_var: config
    file: ci/configs/pipelines.yml
  - across:
    - values: ((.:config.concourse-resources))
      var: resource
    set_pipeline: ((.:resource)) # <------- would still throw a deprecation warning
    file: ci/templates/pipelines/build-concourse-resource.yml
    vars:
      name: ((.:resource))

The set_pipeline line would still get that error and I'm guessing we don't interpolate that line during runtime.

Ah yes that's true. Still getting used to the structure.

Regarding the set_pipeline, it is working, it's just showing a deprecation warning, and also only shows up in the CLI (using fly) and not when I run the pipelines within the web UI. I'm trying to create a setup where I only need to add one main pipeline to add the rest of the configured pipelines in the repository. If this does one day get deprecated and removed I'm afraid I might need to resort to something like a resource reading a pipeline yml file, and returning the same thing with the set_pipeline value filled out to something I want. That would be very workaroundy and hacky. I'd still rather do this than try and introduce another tool / scripts to loop through and configure the pipelines as I would have been doing in concourse itself.

taylorsilva commented 2 years ago

I created an issue to resolve the problem with identifiers for the task/set_pipeline steps. As per Clara's comment, I don't think we'll work on getting put and get steps working with across given the way Concourse currently thinks about resources and inputs to jobs. Leaving this issue open for future feedback. Re-labeling as an enhancement.

geofffranks commented 2 years ago

Thanks!!

Sent from my iPhone

On Oct 8, 2021, at 12:22 PM, Taylor Silva @.***> wrote:

 I created an issue to resolve the problem with identifiers for the task/set_pipeline steps. As per Clara's comment, I don't think we'll work on getting put and get working with across, given the way Concourse currently thinks about resources and inputs to jobs. Leaving this issue open for future feedback. Re-labeling as an enhancement.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.