jenkins-x-apps / jx-app-jacoco Goto Github PK

The source layout should be refactored to make it easier to understand, eg:

• Go code should be moved into an internal package
• Helper scripts should move into scripts
• etc

See also https://github.com/golang-standards/project-layout

Currently the environment variable TEAM_NAMESPACE is read to determine which namespace should be watched for pipelineactivities, defaulting to jx, and the old extension install script expect to set this in turn on the chart via some EXT_TEAM_NAMESPACE variable. I expect that this logic needs to be replaced by something which just reads /var/run/secrets/kubernetes.io/serviceaccount/namespace (discussion).

This code does not look like it would work when jacoco.xml was published to a private repository, as the GH raw URL will be inaccessible without an API token. I believe you need to use this idiom, as alluded to in storage documentation.

(It seems likely that there will be a lot of boilerplate code like this common to many apps processing build metadata. Perhaps some helper functions could be extracted to a common repository for reuse?)

The go routine channel handling, in particular, the usage of the stop channel, does not allow for a clean shutdown of the container/app.

A top level stop channel should be created and used and passed to the go sub routines. Then an event handler for the various shutdown signals should be registered and used to allow a clean shutdown.

During development, one wants to be able to deploy the image w/o having to do a proper release. Let's document how to do this.

Currently, there is a retry around the CRD retrieve and a retry around the CRD update. If the update of the CRD fails, however, with an HTTP 409, the whole fetch+update operation needs to be retried.

• The changelog is created with the wrong version
• The image is released to the internal registry instead of docker.io

Ensure that the Jacoco app is stable with the latest Jenkins X platform versions.

After promoting the app, submit a PR to https://github.com/jenkins-x/jenkins-x-versions with the latest version. This will then kick off a BDD test run. If the test run is a success the version stream will be updated to point to the latest version of the Jacoco app.

Please see Version Streams for further details.

Currently, only a single struct is sent down the done channel. This means only one go routine will receive the done signal. Instead the done channel should be closed at this point.

func setupSignalChannel(done chan struct{}) {
	sigChan := make(chan os.Signal, 1)
	signal.Notify(sigChan, syscall.SIGTERM)

	go func() {
		logger.Info("waiting for shutdown signal in the background")
		<-sigChan
		logger.Info("received SIGTERM signal - initiating shutdown")
		done <- struct{}{}
	}()
}

Each time the app starts, all PipelineActivitity CRDs get replayed and re-processed. This means activities with the jacoco fact get handled again and the already existing fact replaced with a new identical one.

Either we add a check whether the fact already exists or we investigate how to monitor for only new changes.

• build does not have any dependencies. Recompilation does not occur even if files change. Either make build phony or create a proper dependency
• full and related dependencies don't seem to work

For example:

determining the right commit sha for changelog generation (cgyyotyzntqsbmdpdgh1yg)

https://github.com/jenkins-x-apps/jx-app-jacoco/releases/tag/v0.0.116

I've seen the extension processor getting a 404 when retrieving the report from GH pages shortly after the pipeline activity was updated.

My guess is that the GH pages may have a small delay to update after pushing to the corresponding branch in GH repository.

The problem with the current implementation is that it won't retry to query before the 10 minutes watch delay, which is too long. There should be an exponential backoff to retry to retrieve a report in order to generate the facts as soon as the report is available.

On each merge to master a new release is created, for example - https://github.com/jenkins-x-apps/jx-app-jacoco/releases/tag/v0.0.85.

The generated changelog should only contain the changes between the two releases, but currently, it seems all changes from project creation are included.

This could be a problem with jx step changelog. Either there is bug or it is used wrongly.

Instead of being a Kuberenetes controller, the Jacoco App should be changed to be of type Pipeline Extension.

In this model, the App gets called as part of the metapipeline execution where it can contribute to the pipeline, by making changes to the source, eg the pipeline configuration, but also things like pom.xml. This would also allow for example to implement issue #63.

Needs to switch to back off and retry to getting attachments from Github network.

Backoff and retry on updating pipeline activity CRD (in case of contention).

The code should attempt to align the retry strategy used in jx - see https://github.com/jenkins-x/jx/blob/0666a6bc0a80eacdb0820d0b25116ed1b2d4f814/pkg/kube/crds.go#L619

Below you find an example Fact as currently generated by the app. There seem to be several things not quite right:

• The fact id is '0'. In fact, the code never sets it, meaning all facts will have the same id. What is the intention here? Do we want a random id (aka uuid) in order to identify this particular fact at any time or should the id be some sort of hash over the data, so that two facts which are identical in data get the same id?
• The measurement type says "percent", but we are processing counters. Should the type be "count" or we want to calculate percentages?
• We are only processing the top level aggregated counters in these facts. The underlying jacoco report is more detailed. Is it intentional to only look at the aggregate
• ' Instructions-Coverage', 'Complexity-Coverage', etc is imo misleading. It is not "coverage" which we record, but lines "covered". That's an issue in the Fact definition though

facts:
  - factType: jx.coverage
    id: 0
    measurements:
    - measurementType: percent
      measurementValue: 3
      name: Instructions-Coverage
    - measurementType: percent
      measurementValue: 5
      name: Instructions-Missed
    - measurementType: percent
      measurementValue: 8
      name: Instructions-Total
    - measurementType: percent
      measurementValue: 1
      name: Lines-Coverage
    - measurementType: percent
      measurementValue: 2
      name: Lines-Missed
    - measurementType: percent
      measurementValue: 3
      name: Lines-Total
    - measurementType: percent
      measurementValue: 1
      name: Complexity-Coverage
    - measurementType: percent
      measurementValue: 1
      name: Complexity-Missed
    - measurementType: percent
      measurementValue: 2
      name: Complexity-Total
    - measurementType: percent
      measurementValue: 1
      name: Methods-Coverage
    - measurementType: percent
      measurementValue: 1
      name: Methods-Missed
    - measurementType: percent
      measurementValue: 2
      name: Methods-Total
    - measurementType: percent
      measurementValue: 1
      name: Classes-Coverage
    - measurementType: percent
      measurementValue: 0
      name: Classes-Missed
    - measurementType: percent
      measurementValue: 1
      name: Classes-Total
    name: ""
    original:
      mimetype: application/xml
      tags:
      - jacoco.xml
      url: https://raw.githubusercontent.com/hf-bee/spring-boot-test/gh-pages/jenkins-x/jacoco/hf-bee/spring-boot-test/PR-5/1/target/site/jacoco/jacoco.xml
    statements: []
    tags:
    - jacoco

@deanesmith, @pmuir - WDYT?

The readme refers to jx step collect in the Jenkinsfile as a required step for configuring the app. collect is really just a command alias for stash which I think is much more self-explanatory. I think we should use stash.

Maybe we should also make the implicit git flags in this command explicit, mainly to make it easier for the user to connect the dots.

It will be pretty common to fetch a list of Facts from the API, but currently, the API server doesn't support filtering based on fields, so the only way to filter is to use labels. Adding these labels to the generated Facts, we make it easier for other potential components to filter.

This makes development easier. Use something like this to create JX client:

	factory := clients.NewFactory()
	jxClient, _, err := factory.CreateJXClient()
	if err != nil {
		logger.Fatal(err)
	}

The current version of the app requires that the Maven build has the jacoco-maven-plugin configured in its pom.xml. The app then just collects and stores the report.

If the app is installed, the configuration of pom.xml should not be required. jacoco should be run transparently.

The Helm release occurs before the Docker image is built and published.

For example:

Received 201 response: {"saved":true}
[Pipeline] }
[Pipeline] // dir
[Pipeline] dir
Running in /home/jenkins/go/src/github.com/jenkins-x-apps/jx-app-jacoco
[Pipeline] {
[Pipeline] sh
+ cat VERSION
+ docker build -t docker.io/jenkinsxio/jx-app-jacoco:0.0.85 .
Sending build context to Docker daemon  38.41MB
Step 1/5 : FROM scratch
 ---> 
Step 2/5 : EXPOSE 8080
 ---> Using cache
 ---> c2e4b86d4c12
Step 3/5 : ENTRYPOINT /jx-app-jacoco
 ---> Using cache
 ---> 4d401c8b366e
Step 4/5 : COPY ./tmp/ca-certificates.crt /etc/ssl/certs/
 ---> 307d8bcfcdb2
Removing intermediate container fd273e9a213d
Step 5/5 : COPY ./build/ /
 ---> b709b3263687
Removing intermediate container b828714f6368
Successfully built b709b3263687
[Pipeline] sh
+ cat VERSION
+ docker push docker.io/jenkinsxio/jx-app-jacoco:0.0.85
The push refers to a repository [docker.io/jenkinsxio/jx-app-jacoco]
d880d2ced03f: Preparing
9be191bb1453: Preparing
9be191bb1453: Pushed
d880d2ced03f: Pushed
0.0.85: digest: sha256:ebb82e77f3f78572ae5ef103ea4dfadce5e48d7552e3cc21f164d01045f28550 size: 739

See https://github.com/jenkins-x-apps/jx-app-jacoco/blob/master/Jenkinsfile#L64

If the image build fails, you might end up with a released Helm chart which does not work, since there is no matching image. Something like this might have had happened with 0.0.84, where there was a Helm chart, but no matching image.

At the moment kubectl get apps does not list the Jacoco App. This is because there is no associated resource in the helm template for the app.

We have 2 options here:

1. Add a resource template here similar to how some of the other Apps work.
2. Find a way to auto generate an App Resource definition.

Some info logging is currently done using log.Printf which per default will write to stderr. This leads to the fact that the log messages will appear as errors in the logs.

We should introduce a proper logger (logrus!?) and properly handle the logging levels (error|warning|info).

The golint import path has changed. If it is not installed and one runs make check:

make check
go: finding github.com/golang/lint/golint latest
go: finding github.com/golang/lint latest
go: github.com/golang/[email protected]: parsing go.mod: unexpected module path "golang.org/x/lint"
go: error loading module requirements
make: *** [/Users/hardy/work/go/bin/golint] Error 1

For example from an actual pull request:

In the above example the target of the link is empty

Currently NOTES.txt shows the command on how to get the apps ingress which is not useful.
The notes should contain information similar to the one in the README, aka how do I get jacoco reports generated and how can I view them.

The idea is to avoid

COPY ./tmp/ca-certificates.crt /etc/ssl/certs/

and the coressponding code in the Makefile and its cert target.

The reason this exists atm is that the scratch image does not contain certificates.

See jenkins-x/jx#3066

As an app which just acts as a controller, there is no reason to have a Service, much less ingress for that service.