🦜 Support for binary builds 🦜 #381
Conversation
|
@eformat: 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
bot
added
the
needs-rebase
There was a problem hiding this comment.
@eformat reviewed your PR. Many thanks looks great with some minor questions/suggestions. Let me know if you will continue working on this PR or don't have bandwidth in which case we can look at reworking and merging.
Secondly some unit test would be nice to have.
Also this PR requires rebase as few things changed since it was created, but I will focus on getting it merged soon.
Thanks again.
|
|
||
| ```bash | ||
| oc annotate bc/${BUILD_CONFIG_NAME} --overwrite \ | ||
| buildSpecSourceGitUri=${GIT_URL} \ |
There was a problem hiding this comment.
| buildSpecSourceGitUri=${GIT_URL} \ | ||
| buildSpecRevisionGitCommit=${GIT_COMMIT} \ | ||
| buildSpecRevisionGitAuthorName="${GIT_AUTHOR}" | ||
| ``` |
There was a problem hiding this comment.
Also note that we also missing branch / ref in your PR.
|
|
||
| if build.spec.revision is None: | ||
| commit_sha = self._get_revision_from_build(build) | ||
| else: |
There was a problem hiding this comment.
Should we add logic to get commit_sha if one was not resolved from the self._get_revision_from_build() func and not always when the build.spec.revision is not None?
| @@ -191,7 +195,12 @@ def get_metric_from_build(self, build, app, namespace, repo_url): | |||
|
|
|||
| metric.commit_hash = commit_sha | |||
| metric.name = app | |||
| metric.committer = build.spec.revision.git.author.name | |||
|
|
|||
| if build.spec.revision is None: | |||
There was a problem hiding this comment.
Same as above, get it when metric.committer is not valid from the:
metric.committer = self._get_author_from_build(build)
I don't know if such use case is valid when information is missing from build.spec.revision and have generic option to fill that data in the build?
| @@ -264,6 +273,9 @@ def _get_repo_from_build_config(self, build): | |||
| build_config = v1_build_configs.get( | |||
| namespace=build.status.config.namespace, name=build.status.config.name | |||
| ) | |||
| if build.metadata.annotations.buildSpecSourceGitUri: | |||
| return build.metadata.annotations.buildSpecSourceGitUri | |||
There was a problem hiding this comment.
Is there needed validation of URI at some point? e.g. valid git URI regex?
There was a problem hiding this comment.
That is handled in the CommitMetric class and/or the various collector implementations themselves.
| :param build: the Build resource | ||
| :return: revisions as a str or None if not found | ||
| """ | ||
| if build.metadata.annotations.buildSpecSourceGitUri: |
There was a problem hiding this comment.
Is this mistake? Should it be:
if build.metadata.annotations.buildSpecRevisionGitCommit:
I also don't understand the logic:
if something:
return something
return None
This is equivalent to just:
return something
Because:
something = None
if something:
return something
return None
Unless you wanted to return None if string is empty, then check the len(something) as well.
But as on lines 276-277, I would add some validaitons if the revision is valid regex.
| :param build: the Build resource | ||
| :return: author as a str or None if not found | ||
| """ | ||
| if build.metadata.annotations.buildSpecRevisionGitAuthorName: |
There was a problem hiding this comment.
Very similar comment as above with the unnecessary checking if value exists.
Change to resolve dora-metrics#317. The idea is taken from PR dora-metrics#381, however the implementation allows to have minimal required annotations to calculate commit time. It also allows to define custom annotations. Co-authored-by: Mike Hepburn <eformat@gmail.com>
Change to resolve dora-metrics#317. The idea is taken from PR dora-metrics#381, however the implementation allows to have minimal required annotations to calculate commit time. It also allows to define custom annotations. Co-authored-by: Mike Hepburn <eformat@gmail.com>
Change to resolve dora-metrics#317. The idea is taken from PR dora-metrics#381, however the implementation allows to have minimal required annotations to calculate commit time. It also allows to define custom annotations. Co-authored-by: Mike Hepburn <eformat@gmail.com>
Change to resolve dora-metrics#317. The idea is taken from PR dora-metrics#381, however the implementation allows to have minimal required annotations to calculate commit time. It also allows to define custom annotations. Co-authored-by: Mike Hepburn <eformat@gmail.com>
Change to resolve dora-metrics#317. The idea is taken from PR dora-metrics#381, however the implementation allows to have minimal required annotations to calculate commit time. It also allows to define custom annotations. Co-authored-by: Mike Hepburn <eformat@gmail.com>
|
I am closing this PR as it's being already addressed via PR #498 |
Describe the behavior changes introduced in this PR
Initial support for OpenShift binary builds by annotating the build config each time you build (e.g. from a pipeline).
Linked Issues?
resolves #317
Testing Instructions
deploy the committime exporter and test using an openshift binary build. you pipeline must annotate the build config on each build run.
@redhat-cop/mdt