Add commit ID to version output

[mgoerens]

• add a "build" make target, which injects the git commit ID using ldflags
• add the commit ID to the version output
• add the possibility to get the version and commit information in a JSON format

Example output:

[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$ ./chart-verifier version
chart-verifier v1.12.0 <commit: 1c1d51406d0241cbf9683b8dd731eb4bcb706dec>
[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$ ./chart-verifier version --as-data
{"version":"1.12.0","commit":"1c1d51406d0241cbf9683b8dd731eb4bcb706dec"}[mgoerens@mgoerens-thinkpadp1gen3 chart-verifier]$ 

[komish]

@mgoerens thanks for this. The linter is grumpy ^ https://github.com/redhat-certification/chart-verifier/actions/runs/5508530811/jobs/10039939259?pr=370#step:7:154

[mgoerens]

This should be fixed now.

[mgoerens]

Fixes #370

[komish]

Lets use the existing bin target instead of having a separate build target. Also add this to bin_win for now.

I'll collapse those targets at some point in the future, but for now, we can duplicate the logic in both places.

[komish]

Let's preseed this with a value of unknown

[komish]

We can leave the long description out

[komish]

Let's remove this else here. Instead, return nil in the if block after line 56.

E.g. (disregard pseudocode names, just demonstrating structure)

func runVersion(...) ... {
    if asData {
        printAsDataHopefullySuccessfully()
        return nil
    }
    
    printThingsNormally()
    return nil
    }
}

[mgoerens]

Ok, I was thinking about this and was balanced. Good that you have a strong opinion on this, that helps making a decision :)

[komish]

@mgoerens Left a couple of comments.

The CI complaint is an interesting way. Effectively what it's implying is that the chart-verifier version output is used in report generation, and since we've changed its output, we're breaking our fixtures. Normally, I'd say we can update our fixtures, however I think for this case, I think we want to make sure we're backwards compatible with the reports because changing that has some hefty implications.

Please let me know if you need any guidance with this. I'd also recommend you are rebased off of main at any given time before submitting your PR, just to rule out any differences between your branch and main when it comes to CI. It can be a little finicky with differences between a PR and main.

[komish]

Quick note, the problem is two-fold.

1. Your CLI version is no longer using the library at pkg/chartverifier/version.GetVersion. We need to make sure we're using this because this is shared across both the CLI and the library use cases.

2. Our tests are asserting that the version in generated reports matches the version string in the binary that produces them. We're expecting this to look a specific way ("vX.Y.Z") because this is then normalized and used for comparison against the report.

With this in mind, you can take this one of two ways, I think:

a) The easiest thing to do is just to add the commit information as a flag value. In other words, leave chart-verifier version untouched. Add a --commit flag that spits out the commit. Leave --as-data as you have it. This is backwards compatible but also adds the new features, even if isn't the best UX. You might also introduce pkg/chartverifier/version.GetCommit to facilitate this.

b) Update the e2e tests[1] to comply with expectations. Maybe it can use the --as-data value as a dictionary and be more reliable? This is trickier because these can be finicky.

I'm perfectly fine with both options. Your choice!

[1]https://github.com/redhat-certification/chart-verifier/blob/main/tests/tests/functional/chart_test.py#L57-L86

[mgoerens]

The code does still make use of pkg/chartverifier/version.GetVersion

include apiversion "github.com/redhat-certification/chart-verifier/pkg/chartverifier/version"

[...]

var Version = VersionContext{
	Version: apiversion.GetVersion(),
	Commit:  CommitIDLong,
}

I ran make test prior to submitting the PR and was happy to get a green light. I didn't realize their are also a set of python tests ?

I'll look at option a) and b) in details tomorrow.

[komish]

The code does still make use of pkg/chartverifier/version.GetVersion

Aha, missed this. Thanks for pointing it out!

[mgoerens]

All right. Pipeline passes. Though I still don't manage to reproduce it locally, the error seemed to be coming from the lack of newline in the output of version --as-data. I can't understand why, but only some docker tests were failing due to that, and not always the same were failing depending on which one were run. For instance in this pipeline where I commented out the first docker test + all podman and tarball tests, only the 5th and 6th test pass (out of seven). If I run all tests only the first docker test passes, and following docker tests fail (all podman and tarball tests pass regardless). Go figure ... Besides, I cannot reproduce this behavior on a fresh debian instance.

So I added a newline and now all tests pass.

Full list of changes (from commit message):

- use ldflags to injects the git commit ID at build time
- add the commit ID to the version output
- add the possibility to get the version and commit information in a
  JSON format
- adapt the tests to make use of the JSON output
- adapt Dockerfile to make use of the make bin target
- fix path to chart used to test the built docker image in buildandtest
  script (moved as part of !271) and make use of the JSON output

Reg. the last point: the "build-and-test" script is, as the name suggests, comprised of two parts: first it builds the container image, then it tests the image using verify on a known chart. The script is called with the --build-only flag in the pipeline i.e. the image is not tested using this script.

There are multiple issues with the "test" part which makes this script not usable as-is (prior to this PR):

• the chart used for testing has been moved to another location (easy fix)
• the verify command uses a -l flag which isn't supported anymore (easy fix)
• the verify command requires access to a Kubernetes cluster. Currently there is no way to provide Kubernetes credentials to the script (fixable, with some effort)

But before putting further effort into fixing the "test" part of this script, it appears to me like we don't need this script to be able to test a docker image anymore. The pipeline takes care of that in a next step (actually running some extensive tests). AFAICT this script is never called without the --build-only flag in our testing / release process (and anyway it would not have worked, even prior to this PR).

My suggestion is to not fix this part and to consider dropping the "test" part of the buildandtest script altogether in another PR.

[mgoerens]

Opened #378 for the buildandtest issue

[komish]

/lgtm

Thanks @mgoerens!