fly check-resource doesn't report errors to the user

[marco-m]

Bug Report

fly check-resource doesn't report errors to the user

Steps to Reproduce

Consider the pipeline

resources:
- name: booklit
  type: git
  source:
    uri: https://github.com/vito/booklit
    branch: master

jobs:
- name: unit
  plan:
  - get: booklit
    trigger: true

Let's set it

fly -t prod set-pipeline -p fly-bug -c pipeline.yml

At the time of writing, the last commit on https://github.com/vito/booklit is 8efdca98ba233c38dbba96d6e15d1f4d6252dbf6, which is shown in the UI:

If we look at the recent commits, we have:

git log -3 --pretty=tformat:"%H %s"
8efdca98ba233c38dbba96d6e15d1f4d6252dbf6 special-case / to render /index.html
3742983bdaa1073f9162c3ec6b27a2c72469c65a switch to Go plugins, speed up server mode
6ab6216347dc364e6341b528ad86b4d8d7172983 write search index once, at the end

If we pass an existing hash the resource is correctly updated:

> fly -t prod check-resource -r fly-bug/booklit -f ref:6ab6216347dc364e6341b528ad86b4d8d7172983
checked 'booklit'
> echo $status
0

And now for the bug: if I specify a non-existing hash OR anything else, fly check-resource will not report:

1. that there was an error
2. what the error was

Example:

Invalid ref

> fly -t prod check-resource -r fly-bug/booklit -f ref:hello
checked 'booklit'
> echo $status
0

You might consider that the invalid ref is a silly example. The problem is that the same happens for a valid but non existing ref:

fly -t prod check-resource -r fly-bug/booklit -f ref:1234567
checked 'booklit'
> echo $status
0

Expected Results

When passing an invalid or non-existing version, I expected

1. an error message from fly mentioning the cause of the error
2. fly to return non zero exit code

Actual Results

fly doesn't mention any error and the exit code is 0, leaving the user unable to understand if there was an error.

Additional context

Somewhat related #1096

Version Info

• Concourse version: 5.0
• Deployment type (BOSH/Docker/binary):
• Infrastructure/IaaS:
• Browser (if applicable):
• Did this used to work? no

[xtremerui]

Hi, thx reporting this bug. After some investigation, these are what I found:

1. concourse itself is fine since it is replying on the check response from each individual resource to determine if there is an error when running the check command https://github.com/concourse/concourse/blob/master/atc/resource/resource_check.go#L17-L24

2. In this case, the bug is in git-resource that it first check if there is such commit hello or 1234567 by git cat-file -e $ref https://github.com/concourse/git-resource/blob/master/assets/check#L55-L65. So it is an invalid ref, it will just return the latest commit sha. That is why there is no err output from fly check-resource

Maybe creating an issue under git-resource repo instead? Not sure if there is anything we can do with the v2 resource interface.

[marco-m]

@xtremerui thanks for the explanation! Regarding opening an issue under the git-resource, if I am not mistaken a repo admin can actually move the issue from one repo to the other. Maybe @vito ?

[xtremerui]

It seems like it is a desired response from check script based on the discussion we have at retros. Though I think it is still a bit un-clear on resource interface doc here

The list may be empty, if there are no versions available at the source. If the given version is already the latest, an array with that version as the sole entry should be listed.

If your resource is unable to determine which versions are newer then the given version (e.g. if it's a git commit that was push -fed over), then the current version of your resource should be returned (i.e. the new HEAD).

Since in this case if the sha is dummy then the git resource actually returning a latest commit, not an empty list, which makes fly check-resource thinks all good. We might want to make the doc more clear on that @vito ?

And also an issue will be created for having some validation of the ref value. Maybe, resource could still return latest commit and a message indicating the ref is not found at the same time in such way fly would have proper output.