Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concourse CI Pipeline Editor reports errors on valid pipeline.yml #41

Closed
ollema opened this issue Feb 20, 2018 · 5 comments
Closed

Concourse CI Pipeline Editor reports errors on valid pipeline.yml #41

ollema opened this issue Feb 20, 2018 · 5 comments
Milestone

Comments

@ollema
Copy link

ollema commented Feb 20, 2018

Hi!

I've seen this plug-in report lines that are valid pipeline config. What's interesting is that these pipelines are validating and formatted using fly format-pipeline so they should be totally legit.

pipeline.yml:

Both errors I have found were in the same inline task definition:

- task: find-ami
  config:
    platform: linux
    image_resource:
      type: docker-image
      source:
        repository: czero/rootfs
    params:
      REGION: ""
    run:
      path: bash
      args:
      - -c
      - |
        ami=$(grep $REGION pivnet-opsmgr/*.yml | cut -d' ' -f2)
        echo $ami > stock-ami/ami
    inputs:
    - name: pivnet-opsmgr
      path: ""
      optional: false
    outputs:
    - name: stock-ami
      path: ""
  params:
    REGION: {{aws_region}}

Errors:

screen shot 2018-02-20 at 12 00 34

  1. here, the plugin complains the the paths in the input/output sections should not be empty, even though it was set to an empty string by fly format-pipeline and it is totally valid to do so (if the sting is empty, the name of the input/output is assumed to be the path)

  2. here, the plugin complains about the new 'optional' property introduced in 3.9.0, see https://concourse.ci/running-tasks.html#input-optional

  3. is the same as 1

let me know if you need more information!

@kdvolder
Copy link
Member

kdvolder commented Feb 20, 2018

Thanks for the report.

  1. The errors on the empty paths... I kind of did that on purpose, but maybe it was a mistake.

The idea was, that if you bothered to type 'path: ' you probably intended to put a real value in there so if you left it empty it was probably by mistake. Since 'path' is optional, if you don't care about setting it you could simply... not set it, rather than set it to "".

I.e like so:

    inputs:
    - name: pivnet-opsmgr
      optional: false

But seeing as it is formally accepted by fly CLI probably should simply allow empty strings there, or, at least make this a warning rather than an error.

What you think? Warning... or nothing? I still think its odd for someone to have path: "" in their pipeline. So I don't think a warning is really unwarranted there. I understand that fly cli returns these oddities, but perhaps it should be a little smarter and omit them.

  1. Yeah... optional is new since editor schema was created. Unless someone alerts us about these kinds of changes they tend to not make it into editor schema. So thanks for the report.

I'll try and do a fix later this afternoon for both issues (i.e. empty stirng -> warning and add the optional property to schema.).

@ollema
Copy link
Author

ollema commented Feb 20, 2018

Yeah, I can totally see why that error was added! But I agree, since this is something that is produced from the fly format-pipeline, I don't think it should be an error.

I think a warning might be more suitable, maybe something like (for all paths):

If path is not specified or set to an empty string, the input/outputs name is used.

which kind of mimics the docs here:
https://concourse.ci/running-tasks.html#input-path
https://concourse.ci/running-tasks.html#output-path

Or one could maybe skip the warning, and simply rely on the tooltip (wish I could screenshot tooltips, but it says:)

jobs[1].plan[1].do[0].config.inputs[0].path
[String]
Optional. The path where the input will be placed. If not specified, the input's name is used.

Okay, sounds great! Thanks for a super useful plugin :)

kdvolder added a commit that referenced this issue Feb 20, 2018
1) Turn errors for empty task.input|output.path into a warning

2) Add task.optional property to the schema
@kdvolder
Copy link
Member

Its fixed. You can try snapshot build from here: http://dist.springsource.com/snapshot/STS4/nightly-distributions.html

Just download the vscode-concourse vsix file from there. And you can install it into vscode following these instructions: https:/spring-projects/sts4/blob/master/vscode-extensions/vscode-concourse/developer-notes.md#getting-and-installing-latest-snapshot

@ollema
Copy link
Author

ollema commented Feb 21, 2018

cool! I'll have a look tomorrow.

when will this be available in the "official release"?

@martinlippert
Copy link
Member

@olle-brkt the next "official" update will be shipped to the marketplaces as soon as this or next week, aiming for end of this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants