Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🦜 Support for binary builds 🦜 #381

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions charts/pelorus/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ type: application

# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
version: 1.4.6
version: 1.4.7

dependencies:
- name: exporters
version: "v1.3.0"
version: "v1.3.1"
2 changes: 1 addition & 1 deletion charts/pelorus/charts/exporters/Chart.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
name: exporters
version: v1.3.0
version: v1.3.1
apiVersion: v2
2 changes: 1 addition & 1 deletion charts/pelorus/templates/dashboard-sdp-byapp.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ metadata:
name: dashboard-sdp-byapp
labels:
app: grafana
monitoring-key: grafana
spec:
name: devops.json
json: >-
{
"annotations": {
Expand Down
2 changes: 1 addition & 1 deletion charts/pelorus/templates/dashboard-sdp.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ metadata:
name: dashboard-sdp
labels:
app: grafana
monitoring-key: grafana
spec:
name: devops.json
json: >-
{
"annotations": {
Expand Down
21 changes: 21 additions & 0 deletions exporters/committime/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,24 @@ Then we get commit data from the following systems through their respective APIs
* GitHub Enterprise (including private endpoints)
* Bitbucket _(coming soon)_
* Gitlab _(coming soon)_

## Binary Build Support

OpenShift binary builds are a popular mechanism for building container images on OenShift but do not contain source code information.

To support these types of build, you may annotate the build phase with the following annotations for pelorus to use:

| Annotation | Example | Description |
|---------------------------------|:----------------------------------|------------------------------:|
| buildSpecRevisionGitCommit | cae392a | Short or Long Git Commit Hash |
| buildSpecSourceGitUri | https://github.com/org/myapp.git | URL of the Source Repository |
| buildSpecRevisionGitAuthorName | joe.bloggs | Name of the committer |

Example command to put in your build pipeline each time you start an OpenShift `Build`:

```bash
oc annotate bc/${BUILD_CONFIG_NAME} --overwrite \
buildSpecSourceGitUri=${GIT_URL} \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eformat do you think it's worth matching the annotations with similar labels that are result of build process?
Those labels are described here

Here is example:

"io.openshift.build.source-location"
"io.openshift.build.commit.id" 
"io.openshift.build.commit.author"
"io.openshift.build.commit.ref"

buildSpecRevisionGitCommit=${GIT_COMMIT} \
buildSpecRevisionGitAuthorName="${GIT_AUTHOR}"
```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that we also missing branch / ref in your PR.

38 changes: 36 additions & 2 deletions exporters/committime/collector_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,11 @@ def get_metric_from_build(self, build, app, namespace, repo_url):
repo_url = self._get_repo_from_build_config(build)

metric.repo_url = repo_url
commit_sha = build.spec.revision.git.commit

if build.spec.revision is None:
commit_sha = self._get_revision_from_build(build)
else:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

commit_sha = build.spec.revision.git.commit
metric.build_name = build.metadata.name
metric.build_config_name = build.metadata.labels.buildconfig
metric.namespace = build.metadata.namespace
Expand All @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

metric.committer = self._get_author_from_build(build)
else:
metric.committer = build.spec.revision.git.author.name

metric.image_location = build.status.outputDockerImageReference
metric.image_hash = build.status.output.to.imageDigest
# Check the cache for the commit_time, if not call the API
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there needed validation of URI at some point? e.g. valid git URI regex?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is handled in the CommitMetric class and/or the various collector implementations themselves.


if build_config:
if build_config.spec.source.git:
git_uri = str(build_config.spec.source.git.uri)
Expand All @@ -273,3 +285,25 @@ def _get_repo_from_build_config(self, build):
return git_uri + ".git"

return None

def _get_revision_from_build(self, build):
"""
Determines the git revision from the parent BuildConfig
:param build: the Build resource
:return: revisions as a str or None if not found
"""
if build.metadata.annotations.buildSpecSourceGitUri:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return build.metadata.annotations.buildSpecRevisionGitCommit

return None

def _get_author_from_build(self, build):
"""
Determines the git author from the parent BuildConfig
:param build: the Build resource
:return: author as a str or None if not found
"""
if build.metadata.annotations.buildSpecRevisionGitAuthorName:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very similar comment as above with the unnecessary checking if value exists.

return build.metadata.annotations.buildSpecRevisionGitAuthorName

return None
2 changes: 1 addition & 1 deletion exporters/pelorus/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
logging.basicConfig(
format=DEFAULT_LOG_FORMAT, datefmt=DEFAULT_LOG_DATE_FORMAT, level=numeric_level
)
print("Initializing Logger wit LogLevel: %s" % loglevel.upper())
print("Initializing Logger with LogLevel: %s" % loglevel.upper())

# A NamespaceSpec lists namespaces to restrict the search to.
# Use None or an empty list to include all namespaces.
Expand Down