Skip to content
This repository has been archived by the owner on Jul 25, 2023. It is now read-only.

Gitlab prov metadata #31

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

Gitlab prov metadata #31

wants to merge 11 commits into from

Conversation

cristianvasquez
Copy link
Contributor

@cristianvasquez cristianvasquez commented Mar 20, 2023

This pull request is a Gitlab specific step that attaches provenance metadata to subjects of Type Dataset,

It looks like this:

<attachProvMetadata>
  a                  p:Step ;
  code:implementedBy [ a         code:EcmaScriptModule ;
                       code:link <node:barnard59-rdf/metadata.js#appendGitlabProv> ] ;
  code:arguments
                     [ code:name "subjectsWithClass"; code:value void:Dataset] .

It will produce the following data:

@prefix schema: <http://schema.org/> .
@prefix prov: <http://www.w3.org/ns/prov#> .
@prefix bprov: <https://barnard-prov.described.at/> .

<https://example.org/user/pipeline> a bprov:Codebase ;
	bprov:hasPipelines <https://example.org/user/pipeline/-/pipelines> ;
	schema:description "Test pipeline" ;
	schema:name "test-pipeline" .

<https://example.org/user/pipeline/-/pipelines> bprov:contains <https://example.org/user/pipeline/-/pipelines/36212> .

<https://example.org/user/pipeline/-/commit/30dd92dc282586159c8d4401d26262351f7228e0> a bprov:Commit ;
	bprov:triggered <https://example.org/user/pipeline/-/pipelines/36212> ;
	schema:name "I added the commit title" ;
	bprov:author "User <[email protected]>" ;
	prov:atTime "2023-03-14T12:24:50+01:00"^^<http://www.w3.org/2001/XMLSchema#dateTime> .

<https://example.org/user/pipeline/-/pipelines/36212> a bprov:PipelineRun ;
	prov:startedAtTime "2023-03-14T12:24:53+01:00"^^<http://www.w3.org/2001/XMLSchema#dateTime> ;
	bprov:hasJob <https://example.org/user/pipeline/-/jobs/48940> .

<https://example.org/user/pipeline/-/jobs/48940> a bprov:Activity ;
	prov:startedAtTime "2023-03-14T12:25:09+01:00"^^<http://www.w3.org/2001/XMLSchema#dateTime> ;
	prov:wasTriggeredBy <https://example.org/user/pipeline/-/commit/30dd92dc282586159c8d4401d26262351f7228e0> ;
	bprov:hasEnvironment <https://barnard-prov.described.at/environment/develop> .

It's mainly a draft because the model is not yet stable. (I need your help to make it stable)

Copy link
Contributor

@tpluscode tpluscode left a comment

Choose a reason for hiding this comment

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

In addition to comments:

  1. Please add the new step to manifest.ttl
  2. I recommend the use of @tpluscode/rdf-ns-builders overall when working with common vocabs in code

it('provFromGitlab produces a provenance template', async () => {
setMockEnvironment()
const pointer = provFromGitlab()
strictEqual(pointer.dataset.toString(), snapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would propose to use chai and something like chai-snapshot-matcher

And also canonical representation for consistent comparison

Suggested change
strictEqual(pointer.dataset.toString(), snapshot)
expect(pointer.dataset.toCanonica()).to.matchSnapshot(this)

Comment on lines 51 to 53
it('should be a function', () => {
strictEqual(typeof provFromGitlab, 'function')
})
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH, I never understood the utility of those test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it's to ensure the 'intended type' in a test.
For example, in our code-bases to ensure that a factory is actually a factory, or in other cases that a promise is a promise.

It helps when you don't have types, and you want to protect yourself from shooting in the foot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any other test will fail when you try to call this so I find "testing language feature" like that a little pointless

Copy link
Contributor Author

@cristianvasquez cristianvasquez Mar 21, 2023

Choose a reason for hiding this comment

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

I'm just guessing :)
Do you think these need to be removed from barnard-rdf tests ?

test/metadata/produceProv.test.js Outdated Show resolved Hide resolved

export { cube, rdf, rdfs, sh, xsd, _void, dcat, schema, dcterms }
export { cube, rdf, rdfs, sh, xsd, _void, dcat, schema, dcterms, prov }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export { cube, rdf, rdfs, sh, xsd, _void, dcat, schema, dcterms, prov }
export { cube, rdf, rdfs, sh, xsd, _void, dcat, schema, dcterms }
export { prov } from '@tpluscode/rdf-ns-builders'

Might consider for other vocabs too. These will also give you IDE suggestions

lib/appendGitlabProv.js Outdated Show resolved Hide resolved
lib/appendGitlabProv.js Outdated Show resolved Hide resolved
Comment on lines 24 to 28
it('should throw an error if no argument is given', async () => {
await assertThrows(async () => {
await appendGitlabProv()
}, Error, /Needs subjectsWithClass as parameter/)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I think I would not throw but log a warning and continue as pass-through. Not sure though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it needs to stop, so the developer can fix the pipeline.

What I'm not sure about is if warnings should be shown if the pipeline is run outside GitLab.

Copy link
Contributor

Choose a reason for hiding this comment

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

See the other comment where I propose to put this whole thing around.

Metadata is dependent on the environment. You would not have a warning that "it's not GitLab" but rather an info that "GitLab has been detected". And later have the option for other providers, also custom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. 'Gitlab detected' is ok, but this pull request only knows about Gitlab

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the step to 'detect' environments instead of a warning.

It will log when Gitlab variables are detected.

lib/appendGitlabProv.js Outdated Show resolved Hide resolved
test/appendGitlabProv.test.js Outdated Show resolved Hide resolved
test/appendGitlabProv.test.js Outdated Show resolved Hide resolved
@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2023

⚠️ No Changeset found

Latest commit: cb00163

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

Merging #31 (0c98247) into master (46aae93) will increase coverage by 0.26%.
The diff coverage is 98.94%.

❗ Current head 0c98247 differs from pull request most recent head 40560f3. Consider uploading reports for the commit 40560f3 to get more accurate results

@@            Coverage Diff             @@
##           master      zazuko/barnard59-rdf#31      +/-   ##
==========================================
+ Coverage   97.00%   97.26%   +0.26%     
==========================================
  Files          19       21       +2     
  Lines        1168     1355     +187     
==========================================
+ Hits         1133     1318     +185     
- Misses         35       37       +2     
Impacted Files Coverage Δ
lib/appendPipelineProv.js 96.87% <96.87%> (ø)
lib/metadata/produceProv.js 100.00% <100.00%> (ø)
lib/namespaces.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Successfully merging this pull request may close these issues.

3 participants