Skip to content
Snippets Groups Projects

Maven: fix lister after docker-dev review.

Closed Maven: fix lister after docker-dev review.
generated-differential-D7052-source into generated-differential-D7052-target
Closed Boris Baldassari requested to merge generated-differential-D7052-source into generated-differential-D7052-target

Fixing the lister after docker-dev test runs:

  • The last_update entry in ListedOrigins was undef, now returns a datetime object.
  • The lister would produce an invalid origin (https) in the case of malformed scm poms. Now we check the visit_type before yielding scm entries to make sure we only throw valid types.

Test Plan

Added a test case for the invalid scm tag in POM file, as can happen on maven central (e.g. with sprova4j).

Migrated from D7052 (view on Phabricator)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D7052 (id=25578)

    Rebasing onto a599493b...

    Current branch diff-target is up to date.
    Changes applied before test
    commit fa4a3cc81503ecde07be0b7a8b7b4c6dafd88ec1
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:36:57 2022 +0100

    maven: Fix last_update.

commit 7556f6bc555597d947966acc7db257b309d4dc24
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:35:57 2022 +0100

    maven: Fix bug with malformed pom + add tests.

    See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/442/ for more details.

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author
    • Fix last_update tzinfo.
  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D7052 (id=25581)

    Rebasing onto a599493b...

    Current branch diff-target is up to date.
    Changes applied before test
    commit 3838edc7964e3cc5212dfb8ee6ffb853a6697892
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 22:18:05 2022 +0100

    Fix last_update tzinfo.

commit fa4a3cc81503ecde07be0b7a8b7b4c6dafd88ec1
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:36:57 2022 +0100

    maven: Fix last_update.

commit 7556f6bc555597d947966acc7db257b309d4dc24
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:35:57 2022 +0100

    maven: Fix bug with malformed pom + add tests.

    See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/443/ for more details.

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    I'm confused by one of the test code change. And I have some other suggestions inline ;)

    Can you please change the diff's title and description to explain what's wrong and what's being fixed? And then align this within the commit message.

    Could you also please split the diff in 2, something like, one for the:

    • last update fix
    • scm origin management fix

    Thanks in advance.

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    Merge request was returned for changes

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author

    ! In !261 (closed), @ardumont wrote: I'm confused by one of the test code change. And I have some other suggestions inline ;) Explained and fixed the comments.

    Can you please change the diff's title and description to explain what's wrong and what's being fixed? And then align this within the commit message. Fixed for the description. As for the commit messages, would simply rebasing + squashing the last 2 commits into one be ok for you? (in my view the messages do reflect what they fix so not sure what should be done). I've tried to improve and be more precise in the wording.

    Could you also please split the diff in 2, something like, one for the:

    • last update fix
    • scm origin management fix

    Hum. I'm not sure how to do that. These changes happen roughly in the same region of the same file, so it would imply to have the fixes in parallel versions (/branches) of the file, right?

    My intention by having two (well, now three) commits was precisely to distinguish the two fixes. Do you want me to apply the commits on 2 branches so as to create 2 fixes? Or how should I proceed?

    Thanks!

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author
    • maven: dismiss origins if they are malformed - e.g. wrong pom scm format, add test.
    • maven: fix undef last_update in ListedOrigins.
  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build is green

    Patch application report for D7052 (id=25605)

    Rebasing onto a599493b...

    Current branch diff-target is up to date.
    Changes applied before test
    commit 2595d628d596983311dd38ff7c7bc981c1d81700
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:36:57 2022 +0100

    maven: fix undef last_update in ListedOrigins.
    
    maven: Fix last_update tzinfo.
    
    maven: Fix/document last_update test case.

commit e07bffeb5973a32600258d77d200b4e8998e9db0
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:35:57 2022 +0100

    maven: dismiss origins if they are malformed - e.g. wrong pom scm format, add test.

    See https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/444/ for more details.

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    I'm confused by one of the test code change. And I have some other suggestions inline ;) Explained and fixed the comments.

    Thanks

    Can you please change the diff's title and description to explain what's wrong and what's being fixed? And then align this within the commit message. Fixed for the description. As for the commit messages, would simply rebasing + squashing the last 2 commits into one be ok for you? (in my view the messages do reflect what they fix so not sure what should be done). I've tried to improve and be more precise in the wording.

    Great, that's clearer.

    Could you also please split the diff in 2, something like, one for the:

    • last update fix
    • scm origin management fix

    Hum. I'm not sure how to do that.

    Create as much commits you need in the same branch. Then you just need to be consistent with your arc diff use.

    With 3 commits (as you mentioned after) from the head of your master branch (git checkout master):

    arc diff HEAD~3 --head HEAD~2 --update !261   # first commit you did, we keed that diff id (the description will need amending to reflect the commit message though)
arc diff HEAD~2 --head HEAD~   # creates a new diff on top of the previous one with the 2nd commit (diff creation will just use your commit message as description)
arc diff HEAD~                                # create the last diff for the third commit (same about the diff description)

    (then if you need some update on diffs you keep the ranges as is ^ and add the --update D<XYZ> to amend the diff. If you touch one of the base commit, the diff will need an update to keep in sync).

    These changes happen roughly in the same region of the same file, so it would imply to have the fixes in parallel versions (/branches) of the file, right?

    you can but it's not necessary if you do like i just showed you ;)

    My intention by having two (well, now three) commits was precisely to distinguish the two fixes. Do you want me to apply the commits on 2 branches so as to create 2 fixes? Or how should I proceed?

    If it's too much to create the 3 diffs, i'm fine for you to keep the one diff with the multiple changes (still with the 3 separate commits though). The only caveat there is with that approach is that the diff will lose its context once landed (that's a phabricator limitation which expects "one commit one diff" use). It will only show the first commit once landed.

    Thanks!

    Well same ;)

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    lgtm (see my previous comment with answers ;)

    some working suggestion inline.

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    Merge request was accepted

  • Antoine R. Dumont approved this merge request

    approved this merge request

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author

    ! In !261 (closed), @ardumont wrote: Create as much commits you need in the same branch. Then you just need to be consistent with your arc diff use. Oh, ok. Neat. Thanks for advising. Actually there will only be 2 diffs: one for last_udpate, the other for the scm fix. I had rebased the diff in-between to merge the two commits related to last_update, but failed to update my comments.

    Will do that this evening (and also fix the typos as suggested in diff).

  • Antoine R. Dumont
    Antoine R. Dumont @ardumont ·
    Maintainer

    To be clear, it's ok to land now. Once your commits are ok for you. Update the diff here (to sync back your commits' contents with the diff) and then push once and the build's diff is green. That should close this diff.

    Cheers,

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author
    • maven: Fix undef last_update in ListedOrigins.
    • maven: Dismiss origins if they are malformed - e.g. wrong pom scm format, add test.
  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build has FAILED

    Patch application report for D7052 (id=25737)

    Rebasing onto a599493b...

    First, rewinding head to replay your work on top of it...
Applying: gitlab: Allow listing of instances providing multiple vcs_type
Using index info to reconstruct a base tree...
M	swh/lister/gitlab/lister.py
M	swh/lister/gitlab/tests/test_lister.py
Falling back to patching base and 3-way merge...
Auto-merging swh/lister/gitlab/tests/test_lister.py
CONFLICT (content): Merge conflict in swh/lister/gitlab/tests/test_lister.py
Auto-merging swh/lister/gitlab/lister.py
CONFLICT (content): Merge conflict in swh/lister/gitlab/lister.py
Patch failed at 0001 gitlab: Allow listing of instances providing multiple vcs_type

Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

    Rebase failed (ret=1)!

    Could not rebase; Attempt merge onto a599493b...

    Already up to date.
    Changes applied before test
    commit 0f78b32d552a7e21b1ec5b35cbc4be60adac241f
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:36:57 2022 +0100

    maven: Fix undef last_update in ListedOrigins.
           Fix last_update tzinfo.
           Fix/document last_update test case.
           Fix typo in log.

commit e07bffeb5973a32600258d77d200b4e8998e9db0
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:35:57 2022 +0100

    maven: dismiss origins if they are malformed - e.g. wrong pom scm format, add test.

    Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/445/ See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/445/console

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author

    Update commits

  • Software Heritage Continuous Integration for public repositories
    Software Heritage Continuous Integration for public repositories @swh-public-ci ·

    Build has FAILED

    Patch application report for D7052 (id=25738)

    Rebasing onto a599493b...

    Current branch diff-target is up to date.
    Changes applied before test
    commit 0f78b32d552a7e21b1ec5b35cbc4be60adac241f
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:36:57 2022 +0100

    maven: Fix undef last_update in ListedOrigins.
           Fix last_update tzinfo.
           Fix/document last_update test case.
           Fix typo in log.

commit e07bffeb5973a32600258d77d200b4e8998e9db0
Author: Boris Baldassari <boris@chrysalice.org>
Date:   Mon Jan 31 20:35:57 2022 +0100

    maven: dismiss origins if they are malformed - e.g. wrong pom scm format, add test.

    Link to build: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/446/ See console output for more information: https://jenkins.softwareheritage.org/job/DLS/job/tests-on-diff/446/console

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author

    Try to re-trigger the build.

  • Boris Baldassari
    Boris Baldassari @borisbaldassari ·
    Author

    ! In !261 (closed), @ardumont wrote: To be clear, it's ok to land now. Sorry for the latence, and thanks for clarifying.

    Once your commits are ok for you. Update the diff here (to sync back your commits' contents with the diff) and then push once and the build's diff is green. Not sure why ci failed. Seems to be python-specific, maybe a temporary error?

    ImportError: cannot import name 'TempdirFactory' from '_pytest.tmpdir'

    Anyway, I just added the two typo fixes you mentioned. Will try to re-trigger the build, and as soon as the build is green I'll push to master.

    Thanks, cheers!

    Cheers,

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply