Page MenuHomeSoftware Heritage
Differential D1617

Update docs with release artifact and test scenarios
ClosedPublic
Actions

Authored by moranegg on Jun 19 2019, 4:35 PM.

Details

Reviewers
ardumont
zack
Group Reviewers
Reviewers
Maniphest Tasks
T1717: Create test scenario in a persistent location for HAL and for deposit
T1755: Create artifact release when 'releaseNotes' is in metadata
Commits
rDDEP7582336e1f7e: Added release artifact in loading specification
rDDEP3f34909bcd07: Update loading specification review
rDDEPce2ed8b17eaa: Added test scenarios with HAL
Summary

Update loading specification with current state and release artifact

  • review text and update to current state
  • place technical details in a new spec document
  • edit tables
  • add artifact specs from api point of view
  • add release artifact (prepare for implementation)

Closes T1717

Diff Detail

Repository
rDDEP Push deposit
Branch
docs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6734
Build 9407: tox-on-jenkinsJenkins
Build 9406: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ardumont added inline comments.Jun 20 2019, 11:26 AM
docs/tests/tests_HAL.rst
65

There should be some way on hal's side to deal with us down.
Even though, that should not happen, it still could due to 'reasons' (migration comes to mind...).

moranegg updated this revision to Diff 5400.Jun 20 2019, 3:47 PM
moranegg marked 10 inline comments as done.
  • Update specs and test scenarios in docs with D1617 comments
moranegg marked an inline comment as not done.Jun 20 2019, 3:50 PM
moranegg added subscribers: rdicosmo, grouss.
moranegg added inline comments.
docs/specs/spec-loading.rst
40

occurrence isn't a branch in a snapshot?
and occurrence history is obsolete then?

44

this is the way it was, don't mind deleting the _

111

This is the api output, I imagine you know.
Which is the json representation of the artifact.
I can do a table with columns, but I find this output clear.

or maybe when adding the artifact name it is clearer:

snapshot:  {
      "branches": {
          "master": {
              "target": "396b1ff29f7c75a0a3cc36f30e24ff7bae70bb52",
              "target_type": "revision",
              "target_url": "/api/1/revision/396b1ff29f7c75a0a3cc36f30e24ff7bae70bb52/"
          }
      },
      "id": "a3773941561cc557853898773a19c07cfe2efc5a",
      "next_branch": null
  }

I've use this schema on all api artifacts, can you say if this works?

115

The archive is deposited with a set of descriptive metadata, in the CodeMeta vocabulary.
The following CodeMeta terms implies that the artifact is a release:

136

how about, specifying above from where we get the metadata and here change to CodeMeta term

163

good thinking! and you are right... it's not very readable.
Going for:

the deposit revision is not a commit, therefore the revision created is a `synthetic` revision.

188

The distinction between the comitter_date and the date for a deposit should be specified and documented better.
After high level decisions on the legacy software archival.
@rdicosmo

Here the date is committer_date populated from deposit with CodeMeta term publicationDate

190

Here the date is the date (aka. author_date) populated from a deposit with CodeMeta term creationDate.

This is the date used by @grouss.

docs/specs/spec-technical.rst
8

ack.
Where are the archives stored?

9

Yes sure, it is still specs :-D
I will change that to

A client to test the communication with SWORD protocol

because we still need and have a client (swh client) to test the communication.
maybe this should be linked to the SWORD client part.

19

sure.
Can you verify that the content is still accurate?

24

:-)

docs/tests/tests_HAL.rst
63

the bug was that v2 had the swh-id identifier that was generated for v1. I will reword.

65

yes you are right. let's hope it will happen someday:-)

swh-public-ci added a comment.Jun 20 2019, 3:52 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/163/ for more details.

Harbormaster completed remote builds in B6385: Diff 5400.Jun 20 2019, 3:52 PM
moranegg updated this revision to Diff 5426.Jun 21 2019, 4:32 PM
  • Update specs with date mapping
swh-public-ci added a comment.Jun 21 2019, 4:37 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/164/ for more details.

Harbormaster completed remote builds in B6411: Diff 5426.Jun 21 2019, 4:37 PM
ardumont added inline comments.Jun 21 2019, 5:34 PM
docs/specs/spec-loading.rst
40

Yes, obsolete.
They were replaced by snapshot.

origin -> origin-visit -> snapshot -> {revision, release}

moranegg updated this revision to Diff 5515.Jun 26 2019, 3:45 PM
  • Update specs: ammend occurrences into branches
swh-public-ci added a comment.Jun 26 2019, 3:51 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/165/ for more details.

Harbormaster completed remote builds in B6508: Diff 5515.Jun 26 2019, 3:51 PM
moranegg edited the summary of this revision. (Show Details)Jul 1 2019, 3:38 PM
moranegg added a task: T1717: Create test scenario in a persistent location for HAL and for deposit.
ardumont added inline comments.Jul 5 2019, 12:39 PM
docs/specs/spec-loading.rst
111

This is the api output, I imagine you know.

Yes, i just realized that's the main api indeed.

I've use this schema on all api artifacts, can you say if this works?

The description works.

136

works for me ;)

Note that all tables are a bit misaligned.
To align it properly, i'm letting emacs' org-mode feature do it
(using <TAB> key within the table and the alignment automagically sets itself correctly)

152

Maybe take a real id (that's a hash as well).
Because otherwise, it's not consistent with other identifiers mentions elsewhere (just below for example).

165

I tend to try and shorten sentences where i can.
I'd go for a simpler:

The deposit revision is synthetically created.

And then letting the readers understand the synthetic: True on their own.

176

That's a judgment call, i'm not sure we want to add that here.

Also i'd remove this and amend the chapter when legacy software will actually happen.

docs/specs/spec-technical.rst
8

on disk (but not an objstorage) with references in the db.

19

Can you verify that the content is still accurate?

I don't understand.
What must i check?

43

Use - as the previous chapter.

docs/tests/tests_HAL.rst
58

why do you use + on that line here?

zack requested changes to this revision.Jul 8 2019, 2:51 PM
zack added inline comments.
docs/specs/spec-loading.rst
4–7

"this part" ... of what?

Alternative beginning suggestion: "This specification describes the …"

Also, on first use I like to have Software Heritage in full, e.g., "Software Heritage (SWH) archive", so that the acronym is defined for later.

21–315

"Artifacts creation" should be enough here, SWH is kinda implicit anyway.

33

I don't get what this means. "origin_visit" is an internal table name. You probably mean a specific field in that table? Either way, I suggest to use a descriptive name, with the SQL path as additional detail, e.g., "timestamp of the visit (origin_visit.field_name)".

37

minor grammar issue: "at visit" ← not sure what this means

39–40

Are the branches actually optional? Don't we always have at least one release here?

Relatedly: this use case seems similar with package manager listers/loaders, we should compare with what they do and make sure we are consistent. In particular, I'm not sure we have a "master" branch there, most likely we have a "HEAD" branch, pointing to the most recent version at visit time + one branch for each release (the current one + all previous ones).

44–47

I see what you mean here, and I agree with the arrangement. But the description of these two can probably be simpler, how about:

  • revision: synthetic revision pointing to the expanded submitted tarball
  • directory: root directory of the expanded submitted tarball
52–54

active voice is generally preferable to passive voice (this is a general remark that applies to most of the paragraphs in the spec)

152

Seconded.

165

Just remove this sentence. We have already said it's going to be a synthetic revision above, no need to further expand on the matter, IMO.

176

Agreed. Just remove the above paragraph; the subsequent (factual) paragraph is all we need in a spec.

182

I don't understand what "dates in a deposit" mean. Are these fields that are available via SWORD? if so, we should add a sentence specifying that this column should be interpreted in that context.

184

typo: "recieved"

193

typo "targetting"

394–395

don't call this "hash", it's a "Software Heritage Peristent Identifier" (or SWH PID for short), and you can link that text to the PID doc instead of adding a cf. parenthesis

docs/specs/spec-technical.rst
83

better "status changes"

More generally, this should really be a state diagram, rather than a state transition function serialized as a set of lines. No need to add a separate figure for it, but at least a ASCII art state diagram would be nice here.

docs/tests/tests_HAL.rst
1–5

Why does this belong to our repo? swh-deposit is not HAL-specific, if anything it's SWORD specific. So if there're HAL-specific specs, maybe they belong to some HAL-specific website?

This revision now requires changes to proceed.Jul 8 2019, 2:51 PM
moranegg planned changes to this revision.Jul 8 2019, 4:14 PM
moranegg added inline comments.
docs/specs/spec-loading.rst
4–7

I agree. This page is a part of a collection of specifications, this is why it starts that way, but I prefer starting fresh as you suggest also with the full definition of the acronym.

21–315

sure.

33

I kept it from the original specs when there was no snapshots.
Maybe the best solution it to erase the line above.
and change in snaphot the deposit representation to:

snapshotreception of all occurrences (branches)

How does it sound?

37

I agree. see comment above.

39–40

We do not at the moment. Only master.

With this specs we are introducing the concept of release to a deposit.
Only deposits with certain metadata will have another branch for the release.

@ardumont can you comment about package manager?

44–47

ack.

52–54

Can you show me an example, I tried to avoid using We like "We create an origin from the url used in the metadata".
But I can change everything to active with we.

152

ack.

165

sure.

176

Ok. You should know it is really complicating things and I'm pulling my hair on the Legacy Software.
Maybe that's why I've written it ;-)

182

dates in a deposit can be found via SWORD in the header or in the xml metadata.
for example the first two dates are from the SWORD protocol and result in fields in our DB.
The two other dates are sent in the metadata, but are not mandatory.

184

ack.

193

ack.

394–395

good call.

docs/specs/spec-technical.rst
43

ack

83

we already have one but in latex :-/
I'll see what I can do.

docs/tests/tests_HAL.rst
1–5

Today this information is only in my head.
There are no testers on HAL and if it were for them, the users are the testers.
In my opinion, it should be kept somewhere on our side, if we don't want this process to disappear..
Also, at some point we want to automate this tests on with integration tests, so the automated tests should be detailed before implementation.
SWH as an archival service recommends HAL as a way to inject code into the archive, and we need to be sure it works.
If you have a better SWH location for it, I don't mind changing it.

58

oh no. thanks!

moranegg updated this revision to Diff 5713.Jul 9 2019, 4:16 PM
moranegg marked 23 inline comments as done.
  • Update specs: status image and other modifications
moranegg marked 2 inline comments as done.Jul 9 2019, 4:17 PM
moranegg added inline comments.
docs/specs/spec-loading.rst
136

it's very strange, I'm using Atom and not Emacs and I've openned the file in Emacs and it is misaligned.
If I correct it and go back to Atom it is broken again in Atom..

swh-public-ci added a comment.Jul 9 2019, 4:18 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/169/ for more details.

Harbormaster completed remote builds in B6734: Diff 5713.Jul 9 2019, 4:18 PM
ardumont added inline comments.Jul 10 2019, 11:49 AM
docs/specs/spec-loading.rst
39–40

Are the branches actually optional?

Good catch.
No, the snapshot branch is not optional.
There is at least one targetting the revision (the one targetting the tarball's uncompressed root directory).

Don't we always have at least one release here?

In the current state, we don't have release yet, only 1 revision.
That's part of the specification amending morane is trying to push here.
My understanding is that new release is optional, depending on the metadata received from the clients (hal, etc...).

@ardumont can you comment about package manager?

We plan to refactor the deposit loader according to the package manager loader indeed.
For the branch, we chose "master" at the time. But indeed, most of the other use HEAD (which might have been decided after the deposit, don't remember the details exactly).

We can change it in the spec and in the implementation.
But i'm (always) wondering about migration when we discuss such things...
Do we want to update the old swh-ids? (that might be painful communication wise)

212

committer

moranegg updated this revision to Diff 5749.Jul 10 2019, 2:21 PM
moranegg marked 4 inline comments as done.
  • Fix typo
swh-public-ci added a comment.Jul 10 2019, 2:23 PM

Build is green
See https://jenkins.softwareheritage.org/job/DDEP/job/tox/170/ for more details.

Harbormaster completed remote builds in B6771: Diff 5749.Jul 10 2019, 2:23 PM
zack accepted this revision.Jul 11 2019, 2:56 PM
zack added inline comments.
docs/specs/spec-loading.rst
52–54

I'm letting this one go for now. The style change will require quite some effort, and it's more important to land the spec than block on this.

This revision is now accepted and ready to land.Jul 11 2019, 2:56 PM
Closed by commit rDDEPce2ed8b17eaa: Added test scenarios with HAL (authored by moranegg). · Explain WhyJul 11 2019, 4:04 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
docs/
images/
1 line
specs/
441 lines
100 lines
1 line
tests/
67 lines

Diff 5713

View Options

docs/images/status.png

Loading...
View Options

docs/index.rst

Loading...
View Options

docs/specs/spec-loading.rst

Loading...
View Options

docs/specs/spec-technical.rst

Loading...
View Options

docs/specs/specs.rst

Loading...
View Options

docs/tests/tests_HAL.rst

Loading...
About · Developer information · Legal