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

Added support for metadata for job runs #208

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maxses
Copy link

@maxses maxses commented Mar 27, 2024

With the laminarc tag command the build scripts can augment runs with metadata. This can be SCM-oriented information like the used branch or source-version.
The metadata is a list of key-value-pairs. The metadata is automatically shown in the run-view of a job in the web UI.

Each call of laminarc tag can assign one key/value pair.

Example:

laminarc tag $JOBNAME $NUMBER` \
		Branch "$(git -C $WORKSPACE branch --show-current)"

With the `laminarc tag` command the build scripts can augment runs with
metadata. This can be SCM-oriented information like the used branch or
source-version.
The metadata is a list of key/value pairs. The metadata is automatically
shown in the run-view of a job in the web UI.

Each call of `laminarc tag` can assign one key/value pair.

Example:
```
laminarc tag $JOBNAME $NUMBER` \
		Branch "$(git -C $WORKSPACE branch --show-current)"
```
All keys from metainfo of all shown runs is concatenated. So only
relevant colums for metadata is shown.
pkg-config is needed in docker image.

"laminar.conf" has to be copied from "usr/etc/laminar.conf" to
"etc/laminar.conf" in installation directory.
@@ -60,6 +60,7 @@ set(CMAKE_INCLUDE_CURRENT_DIR ON)
set(CMAKE_CXX_STANDARD 17)

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=deprecated-declarations" )
Copy link
Owner

Choose a reason for hiding this comment

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

What caused this? If it is not related to this change it should be dealt with separately. Also consider that only Debug builds get -Werror, so if this is really needed then it should be put in CMAKE_CXX_FLAGS_DEBUG

@@ -142,6 +143,7 @@ set(LAMINARD_CORE_SOURCES
src/run.cpp
src/server.cpp
src/version.cpp
src/json.cpp
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it is necessary to break this out into a separate file - see later comments for explanation


Example:
```
laminarc tag $JOBNAME $NUMBER Branch "$(git -C $WORKSPACE branch --show-current)"
Copy link
Owner

Choose a reason for hiding this comment

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

Since we only intend this to be called from within the run, laminarc can handle reading $JOBNAME and $NUMBER, see for example what set does

return run->tag(key, value);
else
{
LLOG(WARNING, "No active run with ", job, buildNum);
Copy link
Owner

Choose a reason for hiding this comment

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

return false (propagates error to laminarc)

// store metadata into run
bool tag(const std::string& key, const std::string& value);
// Return the meta as JSON text
std::string getMetaDataJsonString();
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think Run should know about JSON at all. Just do

const std::map<std::string, std::string>& metadata() const { return metadata; }

and do the conversion to JSON in the Laminar class with a local method there.

@@ -101,6 +109,8 @@ class Run {
kj::ForkedPromise<void> startedFork;
kj::PromiseFulfillerPair<RunState> finished;
kj::ForkedPromise<RunState> finishedFork;

std::map<std::string, std::string> metaDataMap;
Copy link
Owner

Choose a reason for hiding this comment

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

There is a minor consideration here: what order should the tags show up in the frontend? Using std::map means they will show up alphabetically, but I wonder if it would be preferable for them to show up in the order that they were created? Unfortunately I think that would require using vector<pair<string,string>> and manually checking for duplicates, or using boost::multi_index_container

Copy link
Owner

Choose a reason for hiding this comment

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

Alphabetically is also fine

LLOG(INFO, "Setting tag ", key, value );
metaDataMap[key]=value;
return true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

make it void if it cannot fail

<template v-show="Object.keys(job.metadata).length" v-for="(value, key) in job.metadata">
<dt>{{key}}</dt>
<dd>{{value}}</dd>
</template>
Copy link
Owner

Choose a reason for hiding this comment

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

I think this would look better before / on top of the artifacts

@@ -131,20 +131,25 @@ <h2>{{route.params.name}}</h2>
<canvas id="chartBt"></canvas>
</div>
<div style="grid-column: 1/-1">
<div v-show="false">{{metaDataKeys=["XBranch", "XVersion"]}}</div>
<div v-show="false">{{allJobs=jobsQueued.concat(jobsRunning).concat(jobsRecent)}}</div>
<div v-show="false">{{metaDataKeys=uniteMetadataKeys(allJobs)}}</div>
Copy link
Owner

Choose a reason for hiding this comment

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

I think metadataKeys is best generated in the JS at the time the run list is received, and updated when a new run joins the list

@@ -131,20 +131,25 @@ <h2>{{route.params.name}}</h2>
<canvas id="chartBt"></canvas>
</div>
<div style="grid-column: 1/-1">
<div v-show="false">{{metaDataKeys=["XBranch", "XVersion"]}}</div>
Copy link
Owner

Choose a reason for hiding this comment

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

Remove

<table class="striped">
<thead><tr>
<th><a class="sort" :class="(sort.field=='result'?sort.order:'')" v-on:click="do_sort('result')">&nbsp;</a></th>
<th>Run <a class="sort" :class="(sort.field=='number'?sort.order:'')" v-on:click="do_sort('number')">&nbsp;</a></th>
<th class="text-center">Started <a class="sort" :class="(sort.field=='started'?sort.order:'')" v-on:click="do_sort('started')">&nbsp;</a></th>
<th class="text-center">Duration <a class="sort" :class="(sort.field=='duration'?sort.order:'')" v-on:click="do_sort('duration')">&nbsp;</a></th>
<th class="text-center vp-sm-hide">Reason <a class="sort" :class="(sort.field=='reason'?sort.order:'')" v-on:click="do_sort('reason')">&nbsp;</a></th>
<th class="text-center vp-sm-hide" v-for="(value, key) in metaDataKeys">{{key}}<a class="sort" :class="(sort.field=='reason'?sort.order:'')" v-on:click="do_sort('reason')">&nbsp;</a></th>
Copy link
Owner

Choose a reason for hiding this comment

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

Do not sort by reason. It should be pretty simple to add sorting by metadata fields, using something like 'meta_' + key for sort.field and in the backend using ORDER BY metadata -> $key, but also happy just to not implement sorting for metadata

@@ -34,6 +34,8 @@ Depends: libcapnp-0.7.0, libsqlite3-0, zlib1g
Description: Lightweight Continuous Integration Service
EOF
echo /etc/laminar.conf > laminar/DEBIAN/conffiles
mkdir -p laminar/etc
mv laminar/usr/etc/laminar.conf laminar/etc/laminar.conf
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is a bug introduced elsewhere and this is not the right place to fix it

Copy link
Owner

Choose a reason for hiding this comment

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

Fixed here 8c3d7f6

@@ -8,7 +8,7 @@ VERSION=$(cd "$SOURCE_DIR" && git describe --tags --abbrev=8 --dirty)-1~upstream

DOCKER_TAG=$(docker build -q - <<EOS
FROM debian:11-slim
RUN apt-get update && apt-get install -y wget cmake g++ capnproto libcapnp-dev rapidjson-dev libsqlite3-dev libboost-dev zlib1g-dev
RUN apt-get update && apt-get install -y wget cmake g++ capnproto libcapnp-dev rapidjson-dev libsqlite3-dev libboost-dev zlib1g-dev pkg-config
Copy link
Owner

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants