Tekton compatibility for committime exporter #333

KevinMGranger · 2022-01-04T20:07:13Z

copied from #327

Describe the behavior changes introduced in this PR

Adds the ability to get git commit information from Tekton runs.

It's a good enough implementation for "technology preview" status. There are open questions about how to extract the right information when there's near infinite flexibility in making a pipeline.

Right now, it tries to follow the (I believe) most common usage patterns:

Looks up git information from the git-checkout ClusterTask that comes with Tekton
Gets the image hash from the first TaskRun with a taskResult named IMAGE_DIGEST
Gets the image location from the first TaskRun with IMAGE or IMAGE_NAME as a param, following patterns from various s2i and buildah tasks

I'm sure we'll need to figure out a way to add some flexibility. I've started exploring some possible solutions while experimenting with this, but that should come later.

Special thanks to @jlozano-rh for starting the work on this.

Linked Issues?

resolves #224

Testing Instructions

Fork my fork of this repo and check out the tekton branch.
Set up the committime exporter. Make sure it is pointed to your fork. The deploytime exporter must also be there but can still point upstream.
Run the tekton demo with ./demo/demo-tekton $URL_FOR_FORK.
Make sure the pipeline was successful and look at the sample page.
Use the prompt to make a change, and wait for it to show up.
Verify the metrics show up in the dashboard.

@redhat-cop/mdt

Remaining Work

feature-flag the tekton features
resolve demo issue: occasional unable to recognize no matches for kind "Pipeline" in version "tekton.dev/v1beta1"
demo should poll for the page to be ready
git-clone task unavailable for the first run?

exporters/committime/collector_base.py

etsauer · 2022-01-05T19:28:54Z

exporters/committime/collector_base.py

+                    git_host = build.metadata.labels.pelorusgithost
+                    git_project = build.metadata.labels.pelorusgitproject
+                    git_app = build.metadata.labels.pelorusgitapp
+                    git_protocol = build.metadata.labels.pelorusgitprotocol
+                    repo_url = (
+                        git_protocol
+                        + "://"
+                        + git_host
+                        + "/"
+                        + git_project
+                        + "/"
+                        + git_app
+                    )


I don't think we should break up git URLs in this way. There are a myriad of supported git url formats that are not covered by this structure, and to try to cover them all would be cumbersome and unnecessary. I would just stick to the two standard git parameters that are used all over the place:

git_url - The full URL string in any format

git_ref - The branch or tag to be examined, if applicable

@etsauer I initially approached the solution using labels. But labels have character restrictions for the value part. The idea is to improve this with an annotation to avoid dirty code like this one.

Using annotations makes sense. I'll add a BUILD_GIT_REPO_URI_ANNOTATION env variable (or is that too long...)

We should probably use a namespaced default, right? Something like pelorus.konveyor.io/git-uri? Or keep it simple with gitUri?

@KevinMGranger I was updating these values with a pipeline dynamically. Regarding the env variable, I think it is fine, I prefer it to be long and understandable rather than short and confusing.

Regarding the namespaced default, I think it's the same thing... keeping it simple can lead to confusion, I think having pelorus word in the name would indicate what it is.

exporters/committime/collector_base.py

mpryc

Just few nits around shell to get started and understand what's happening in the project :)

demo/demo-tekton

demo/python-example/example.py

KevinMGranger · 2022-02-10T15:43:05Z

exporters/committime/collector_base.py

+                pipeline_runs = pelorus.NoOpResourceInstance()
+
+            # use a jsonpath expression to find all possible values for the app label
+            jsonpath_str = f"$['items'][*]['metadata']['labels']['{app_label}']"


This will break if app_label has single quotes in it for some reason. We could extract this part with plain python.

mpryc

I think we should allow to install tkn and oc commands using Makefile dev-env to a separate PATH. This can however be different PR.
Should the project template be compatible as well with kubectl ? It means to use in the demo/tekton-demo-setup/02-project.yaml api version v1, similarly to:
https://github.com/redhat-cop/openshift-applier/blob/main/tests/files/templates/projectrequest.yml

charts/pelorus/charts/exporters/templates/rbac.yaml

exporters/committime/collector_base.py

mpryc · 2022-02-24T12:09:21Z

exporters/committime/collector_base.py

+        Used to find the TaskRun that did the git checkout.
+        """
+        pipeline_resource = self._kube_client.resources.get(
+            api_version="tekton.dev/v1beta1", kind="Pipeline"


As in previous comment. I do think such api_versions should be hidden in the function body.

exporters/committime/collector_base.py

mpryc · 2022-02-24T12:15:59Z

exporters/pelorus/utils.py

+        else:
+            return NotImplemented
+
+    def __repr__(self):


Should you define __str__(self) as well?

I need to double check how these are used within the openshift library. I think I was unsure if __str__ should transparently use the underlying string, or use __repr__.

mpryc · 2022-02-24T14:20:03Z

Few more issues found with the script (using bash as shell on Fedora):

/usr/bin/which: invalid option -- 's'
/usr/bin/oc
/usr/bin/which: invalid option -- 's'
/usr/local/bin/tkn
Setting up resources:
1. Installing tekton operator
2. Setting up python tekton project
Project already exists
3. Setting up build and deployment information
imagestream.image.openshift.io/basic-python-tekton unchanged
persistentvolumeclaim/basic-python-tekton-build-pvc unchanged
pipeline.tekton.dev/basic-python-tekton-pipeline unchanged
service/basic-python-tekton unchanged
route.route.openshift.io/basic-python-tekton unchanged
deploymentconfig.apps.openshift.io/basic-python-tekton configured
./demo/demo-tekton: line 41: $1: unbound variable

KevinMGranger

Addressed your comments, looks like I have a little more work to do.

exporters/committime/collector_base.py

charts/pelorus/charts/exporters/templates/rbac.yaml

exporters/committime/collector_base.py

KevinMGranger · 2022-02-24T15:30:13Z

exporters/committime/collector_base.py

+                commit_sha = spec.revision.git.commit
+                metric.committer = spec.revision.git.author.name
+            else:
+                # TODO: check for existence, allow user customization? Annotation instead?


Good call. I missed the UNKNOWN_AUTHOR part as well-- I think we can just leave it out, but I need to double check Grafana/Prometheus.

exporters/committime/collector_base.py

KevinMGranger · 2022-02-24T15:35:38Z

exporters/pelorus/utils.py

+        else:
+            return NotImplemented
+
+    def __repr__(self):


I need to double check how these are used within the openshift library. I think I was unsure if __str__ should transparently use the underlying string, or use __repr__.

KevinMGranger · 2022-03-02T16:41:01Z

To summarize the remaining work:

Move resource versions / names up into constants
double-check how typedstrings would be interpreted within the openshift library
handle string interpolation in the jsonpath expression

copied from dora-metrics#327

Look for tekton data in standard locations. Add various utility / helper functions.

While we should make a broader error management strategy, this is here while tekton support is in preview.

Also fix referring to wrong FQDN attribute name

…variables

- use TypedStrings where possible to avoid previous errors - filter to only use successful pipeline runs - Rename many variables to be more clear and semantically correct - Avoid passing around names when already present in objects to reduce errors

…data

openshift-ci · 2022-03-16T11:59:11Z

@KevinMGranger: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2022-04-06T15:24:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KevinMGranger

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [KevinMGranger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-06-08T14:28:42Z

@KevinMGranger: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.10-unit	`a721ee3`	link	true	`/test 4.10-unit`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

KevinMGranger · 2022-06-21T18:55:30Z

Because tekton pipelines could be customized to do anything, supporting this by guessing isn't worth the technical cost.

We've decided that the best course of action is to annotate the resulting build with the required info, which was implemented with PR #498. It also solves the problem in issue #317 at the same time!

What would make tekton "support" easier is documenting how you might label the resulting build, and including an example of that. We already have demo code in the repo, so we'll likely use that.

KevinMGranger self-assigned this Jan 4, 2022

KevinMGranger added the committime-exporter Related to the committime exporter label Jan 4, 2022

KevinMGranger added this to the v1.5-rc1 milestone Jan 4, 2022

KevinMGranger assigned KevinMGranger and unassigned KevinMGranger Jan 4, 2022

KevinMGranger commented Jan 4, 2022

View reviewed changes

exporters/committime/collector_base.py Outdated Show resolved Hide resolved

exporters/committime/collector_base.py Outdated Show resolved Hide resolved

exporters/committime/collector_base.py Outdated Show resolved Hide resolved

etsauer reviewed Jan 5, 2022

View reviewed changes

exporters/committime/collector_base.py Outdated Show resolved Hide resolved

KevinMGranger force-pushed the tekton branch from d13e896 to 5678713 Compare February 3, 2022 01:52

mpryc reviewed Feb 8, 2022

View reviewed changes

demo/demo-tekton Outdated Show resolved Hide resolved

demo/demo-tekton Outdated Show resolved Hide resolved

demo/demo-tekton Outdated Show resolved Hide resolved

demo/python-example/example.py Outdated Show resolved Hide resolved

KevinMGranger force-pushed the tekton branch from d37ba56 to 99f6cc1 Compare February 9, 2022 19:20

KevinMGranger mentioned this pull request Feb 9, 2022

Add python tekton pipeline demo #354

Merged

KevinMGranger force-pushed the tekton branch 2 times, most recently from edb82b9 to 2696ebc Compare February 10, 2022 15:28

KevinMGranger commented Feb 10, 2022

View reviewed changes

KevinMGranger force-pushed the tekton branch 4 times, most recently from f4f843b to d4660f1 Compare February 14, 2022 21:49

KevinMGranger marked this pull request as ready for review February 14, 2022 21:49

KevinMGranger changed the title ~~[WIP] Tekton compatibility for committime exporter~~ Tekton compatibility for committime exporter Feb 14, 2022

mpryc reviewed Feb 24, 2022

View reviewed changes

KevinMGranger commented Feb 24, 2022

View reviewed changes

KevinMGranger marked this pull request as draft February 24, 2022 15:36

jlozano-rh and others added 2 commits March 2, 2022 12:01

First pass of tekton compatibility for committime exporter,

5d6dac7

copied from dora-metrics#327

Refactor committime collector base for readability.

f41f7a4

Look for tekton data in standard locations. Add various utility / helper functions.

KevinMGranger added 10 commits March 2, 2022 12:01

Handle any exceptions that bubble up from tekton

37cc7d5

While we should make a broader error management strategy, this is here while tekton support is in preview.

Skip git hooks when running demo and push to URL instead

a401063

Log error message from GitHub response if present

ab424f7

Also fix referring to wrong FQDN attribute name

Grant exporter service account access to tekton resources

3476a1c

Add TypedString utility to reduce mixups with similarly-named string …

b00a207

…variables

Many tekton changes:

2ac28cc

- use TypedStrings where possible to avoid previous errors - filter to only use successful pipeline runs - Rename many variables to be more clear and semantically correct - Avoid passing around names when already present in objects to reduce errors

Document which fields of the CommitMetric are actually used

95399a5

Document methods, remove code collecting some unnecessary commit meta…

60d88fc

…data

Move relevant resources to constants in committime base

248b540

Make apps and pipeline run app labels more resilient

76f7655

KevinMGranger force-pushed the tekton branch from 1f8c3b1 to 76f7655 Compare March 2, 2022 17:11

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 16, 2022

Do committime label gathering in python, not jsonpath

a721ee3

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2022

KevinMGranger mentioned this pull request May 31, 2022

Research generic commit time to raise the level of certainty to ditch tekton specific support #478

Closed

KevinMGranger closed this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tekton compatibility for committime exporter #333

Tekton compatibility for committime exporter #333

KevinMGranger commented Jan 4, 2022 •

edited

Loading

etsauer Jan 5, 2022

jlozano-rh Jan 5, 2022

KevinMGranger Jan 5, 2022

jlozano-rh Jan 6, 2022

mpryc left a comment

KevinMGranger Feb 10, 2022

mpryc left a comment

mpryc Feb 24, 2022

mpryc Feb 24, 2022

KevinMGranger Feb 24, 2022

mpryc commented Feb 24, 2022

KevinMGranger left a comment

KevinMGranger Feb 24, 2022

KevinMGranger Feb 24, 2022

KevinMGranger commented Mar 2, 2022 •

edited

Loading

openshift-ci bot commented Mar 16, 2022

openshift-ci bot commented Apr 6, 2022

openshift-ci bot commented Jun 8, 2022

KevinMGranger commented Jun 21, 2022

Tekton compatibility for committime exporter #333

Tekton compatibility for committime exporter #333

Conversation

KevinMGranger commented Jan 4, 2022 • edited Loading

Describe the behavior changes introduced in this PR

Linked Issues?

Testing Instructions

Remaining Work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpryc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpryc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpryc commented Feb 24, 2022

KevinMGranger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinMGranger commented Mar 2, 2022 • edited Loading

openshift-ci bot commented Mar 16, 2022

openshift-ci bot commented Apr 6, 2022

openshift-ci bot commented Jun 8, 2022

KevinMGranger commented Jun 21, 2022

KevinMGranger commented Jan 4, 2022 •

edited

Loading

KevinMGranger commented Mar 2, 2022 •

edited

Loading