Page MenuHomeSoftware Heritage
Differential D8002

CffMapping: Ignores invalid yaml files
ClosedPublic
Actions

Authored by VickyMerzOwn on Jun 17 2022, 10:46 AM.

Details

Reviewers
vlorentz
KShivendu
Maniphest Tasks
T4276: CffMapping: ignore invalid yaml files
Commits
rDCIDX153bda12aa0d: cff: Ignore invalid yaml files
Summary

Fixes T4276

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

VickyMerzOwn created this revision.Jun 17 2022, 10:46 AM
swh-public-ci added a comment.Jun 17 2022, 10:51 AM

Build is green

Patch application report for D8002 (id=28834)

Rebasing onto 62db0cb208...

First, rewinding head to replay your work on top of it...
Applying: Ignores invalid yaml files
Using index info to reconstruct a base tree...
M	swh/indexer/metadata_dictionary/cff.py
Falling back to patching base and 3-way merge...
Auto-merging swh/indexer/metadata_dictionary/cff.py
CONFLICT (content): Merge conflict in swh/indexer/metadata_dictionary/cff.py
Patch failed at 0001 Ignores invalid yaml files

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 62db0cb208...



Already up to date.





Changes applied before test



commit 2064b713c5d10533d11f98105f568baf037838cf
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/260/ for more details.
Harbormaster completed remote builds in B29920: Diff 28834.Jun 17 2022, 10:51 AM
VickyMerzOwn requested review of this revision.Jun 17 2022, 10:51 AM
ardumont added a subscriber: ardumont.Jun 17 2022, 10:56 AM
ardumont added inline comments.
swh/indexer/metadata_dictionary/cff.py


29
Add a warning log to explicit that the yaml is invalid and that we skip it.
VickyMerzOwn updated this revision to Diff 28836.Jun 17 2022, 11:17 AM
Logs warning of invalid yaml file
swh-public-ci added a comment.Jun 17 2022, 11:22 AM
Build has FAILED



Patch application report for D8002 (id=28836)



Could not rebase; Attempt merge onto 62db0cb208...



Updating 62db0cb..b49dd61
Fast-forward
 swh/indexer/metadata_dictionary/cff.py | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)





Changes applied before test



commit b49dd61f0a1d5ee43c2441f1e8a01c516be85a0f
Merge: e91959e 62db0cb
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:31:19 2022 +0530

    Fix merge conflicts

commit e91959efd7739177601e9ec599b6dd701bfd71e6
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/261/

See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/261/console
Harbormaster failed remote builds in B29921: Diff 28836!Jun 17 2022, 11:22 AM
vlorentz added inline comments.Jun 17 2022, 11:22 AM
swh/indexer/metadata_dictionary/cff.py


29
I disagree, there is nothing we can do about them, so no point in logging. We also do not warn about syntax errors in JSON files.
VickyMerzOwn updated this revision to Diff 28837.Jun 17 2022, 11:34 AM
Logs the invalid-yaml-file-warning
swh-public-ci added a comment.Jun 17 2022, 11:40 AM
Build has FAILED



Patch application report for D8002 (id=28837)



Rebasing onto 62db0cb208...



Current branch diff-target is up to date.





Changes applied before test



commit 93f8b4c9a78f6afdce4ea0345ae8cae9eeb945d3
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/262/

See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/262/console
Harbormaster failed remote builds in B29922: Diff 28837!Jun 17 2022, 11:40 AM
ardumont added inline comments.Jun 17 2022, 11:58 AM
swh/indexer/metadata_dictionary/cff.py


29
to have a tendency or a rough stats of how often that happens, isn't it worth it?

Either way, fine.


39
well, with some more info though ;)

But like i said, i won't push too much.
VickyMerzOwn updated this revision to Diff 28845.Jun 17 2022, 6:53 PM
Adds test for skipping invalid yaml files
swh-public-ci added a comment.Jun 17 2022, 6:59 PM
Build is green



Patch application report for D8002 (id=28845)



Could not rebase; Attempt merge onto 62db0cb208...



Updating 62db0cb..5cb2edf
Fast-forward
 swh/indexer/metadata_dictionary/cff.py | 15 +++++---
 swh/indexer/metadata_dictionary/npm.py | 67 ----------------------------------
 swh/indexer/tests/test_metadata.py     | 27 +++++++++++++-
 3 files changed, 36 insertions(+), 73 deletions(-)





Changes applied before test



commit 5cb2edf23d7b6aa078230d83ea79214fe69ee04d
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 22:22:40 2022 +0530

    Test for invalid yaml file

commit 93f8b4c9a78f6afdce4ea0345ae8cae9eeb945d3
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/263/ for more details.
Harbormaster completed remote builds in B29930: Diff 28845.Jun 17 2022, 6:59 PM
VickyMerzOwn updated this revision to Diff 28846.Jun 17 2022, 11:30 PM
Undo deletion of normalize_description
swh-public-ci added a comment.Jun 17 2022, 11:36 PM
Build is green



Patch application report for D8002 (id=28846)



Could not rebase; Attempt merge onto 62db0cb208...



Updating 62db0cb..22787ea
Fast-forward
 swh/indexer/metadata_dictionary/cff.py | 15 ++++++++++-----
 swh/indexer/metadata_dictionary/npm.py |  3 +--
 swh/indexer/tests/test_metadata.py     | 25 +++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 7 deletions(-)





Changes applied before test



commit 22787ea2cf4efc49716c622f9ad19e21b5803de0
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 22:22:40 2022 +0530

    Test for invalid yaml file
    
    to find the diff
    
    Hypothesis falsification overcome

commit 93f8b4c9a78f6afdce4ea0345ae8cae9eeb945d3
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/264/ for more details.
Harbormaster completed remote builds in B29931: Diff 28846.Jun 17 2022, 11:36 PM
VickyMerzOwn updated this revision to Diff 28847.Jun 20 2022, 10:04 AM
    

  • Ignores invalid yaml files
    • 

  • Solves hypothesis error in T4333
    • 

swh-public-ci added a comment.Jun 20 2022, 10:06 AM
Build has FAILED



Patch application report for D8002 (id=28847)



Rebasing onto 62db0cb208...



Current branch diff-target is up to date.





Changes applied before test



commit f817b1a6ae5266a23188c1f8e7ea80f5daf6a0c5
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 22:22:40 2022 +0530

    Solves hypothesis error in T4333

commit 93f8b4c9a78f6afdce4ea0345ae8cae9eeb945d3
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/265/

See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/265/console
Harbormaster failed remote builds in B29932: Diff 28847!Jun 20 2022, 10:06 AM
VickyMerzOwn updated this revision to Diff 28848.EditedJun 20 2022, 10:23 AM
Rebase
swh-public-ci added a comment.Jun 20 2022, 10:28 AM
Build is green



Patch application report for D8002 (id=28848)



Rebasing onto 14f8a8574c...



First, rewinding head to replay your work on top of it...
Applying: Ignores invalid yaml files
Applying: Solves hypothesis error in T4333





Changes applied before test



commit 75a038fee74e7eb454bc06c2a0f80d2dd379b5cc
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 22:22:40 2022 +0530

    Solves hypothesis error in T4333

commit b6254f9e0496e6a314e6e249a8cee4a11c1f0f42
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    
    Adds warning log that yaml is invalid



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/266/ for more details.
Harbormaster completed remote builds in B29934: Diff 28848.Jun 20 2022, 10:28 AM
vlorentz added a comment.Jun 20 2022, 10:35 AM
The code looks good, though I have two inline comments below.



About the diff itself:



    

  1. Please squash the two commits
    2. 

  2. Please mention the task you are solving in the diff description (see https://docs.softwareheritage.org/devel/contributing/git-style-guide.html )
    3. 

  3. also reword the commit message to follow the Git style conventions (see the linked doc, too)
    4. 

  4. when updating diffs, please describe your changes (eg. "rebase" when rebasing, "fix XXX" when fixing a bug that you introduced, "add missing commit", etc., instead of repeating
    5. 




Ignores invalid yaml files
Solves hypothesis error in T4333



because this is meant for reviewers to understand what you changed without digging in the diff history.
swh/indexer/metadata_dictionary/npm.py


138–162 ↗(On Diff #28848)
Please undo these changes for now, they are out of scope of this diff. Then, please submit another diff with only this change.


swh/indexer/tests/test_metadata.py


166–175
you can simplify this example, to only keep what matters to the test. It makes the test more readable. (Right now, I don't see what part of this file is the syntax error)
VickyMerzOwn added inline comments.Jun 20 2022, 10:51 AM
swh/indexer/tests/test_metadata.py


166–175
ok, I'll take the unnecessary part out. Would you like me to add a comment describing what is wrong in content?
VickyMerzOwn updated this revision to Diff 28849.EditedJun 20 2022, 11:33 AM
Cleans commit history



    

  1. Updating D8002: Ignores invalid yaml files
    2. 

  2. Solves T4276.
    3. 

  3. Adds a try except statement to catch yaml parser exceptions.
    4. 

  4. Tests using an invalid example which must return None.
    5. 

swh-public-ci added a comment.Jun 20 2022, 11:38 AM
Build is green



Patch application report for D8002 (id=28849)



Rebasing onto 14f8a8574c...



First, rewinding head to replay your work on top of it...
Applying: Ignores invalid yaml files





Changes applied before test



commit 84a0d6c6ac8776d46818e01d47d25e11c513ef99
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    Tests that invalid yaml files are skipped



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/267/ for more details.
Harbormaster completed remote builds in B29935: Diff 28849.Jun 20 2022, 11:38 AM
vlorentz added a comment.Jun 20 2022, 2:09 PM


In D8002#208144, @VickyMerzOwn wrote:


    

  1. Updating D8002: Ignores invalid yaml files
    2. 

  2. Solves T4276.
    3. 






you should write these in the commit message, not in the update
swh/indexer/metadata_dictionary/npm.py


138–139 ↗(On Diff #28849)
you forgot to undo this change too


swh/indexer/tests/test_metadata.py


166–175
yes, please
VickyMerzOwn updated this revision to Diff 28850.Jun 20 2022, 3:10 PM
Undo a change not for T4276
swh-public-ci added a comment.Jun 20 2022, 3:16 PM
Build is green



Patch application report for D8002 (id=28850)



Rebasing onto 14f8a8574c...



First, rewinding head to replay your work on top of it...
Applying: Ignores invalid yaml files





Changes applied before test



commit 57fded1b9d812aa61d601dac5ffb3c9886da5b85
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    Ignores invalid yaml files
    Tests that invalid yaml files are skipped



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/268/ for more details.
Harbormaster completed remote builds in B29936: Diff 28850.Jun 20 2022, 3:16 PM
vlorentz added a comment.Jun 21 2022, 7:56 AM


In D8002#208133, @vlorentz wrote:


    

  1. Please mention the task you are solving in the diff description (see https://docs.softwareheritage.org/devel/contributing/git-style-guide.html )
    2. 

  2. also reword the commit message to follow the Git style conventions (see the linked doc, too)
    3. 






You forgot about these
VickyMerzOwn added a comment.Jun 21 2022, 8:13 AM





In D8002#208133, @vlorentz wrote:


    

  1. Please mention the task you are solving in the diff description (see https://docs.softwareheritage.org/devel/contributing/git-style-guide.html )
    2. 






Do I repeat which task I am solving in every diff description?

If so, I suppose I am to put as the subject, what I am changing in the diff, eg. "rebase", "fix XXX" etc in the title of the diff. Is this right?



    

  1. also reword the commit message to follow the Git style conventions (see the linked doc, too)
    2. 




So my commit message will be "Fixes T4276" only?

Wouldn't I need to mention that a test to verify if the fix works is also added in this commit?
vlorentz added a comment.Jun 21 2022, 8:34 AM


In D8002#208179, @VickyMerzOwn wrote:




In D8002#208133, @vlorentz wrote:


    

  1. Please mention the task you are solving in the diff description (see https://docs.softwareheritage.org/devel/contributing/git-style-guide.html )
    2. 






Do I repeat which task I am solving in every diff description?





Yes



If so, I suppose I am to put as the subject, what I am changing in the diff, eg. "rebase", "fix XXX" etc in the title of the diff. Is this right?



No, the title should describe the change, so it is readable without opening the diff or the task. "fix XXX" should only be a line within the description. "rebase" would never be in the title of a diff; I think you confuse diffs (eg. D8002 is a diff, created with arc diff --create or simply arc diff with no --update argument) with diff updates (which are a change to an existing diff, created with arc diff --update)



    

  1. also reword the commit message to follow the Git style conventions (see the linked doc, too)
    2. 




So my commit message will be "Fixes T4276" only?



It should be "cff: Ignore invalid yaml files" because:



    

  1. imperative mood
    2. 

  2. the "cff:" prefix makes it clearer what part of the code is changed
    3. 

  3. "Ignore invalid yaml" instead of "Fix T4276", because it is meant to be read without jumping between web pages. Additionally, git histories usually outlive forges, so we should not rely on the forge to keep this type of information.
    4. 




Wouldn't I need to mention that a test to verify if the fix works is also added in this commit?



It is implicit that every fix comes with a new test.
VickyMerzOwn retitled this revision from Ignores invalid yaml files to CffMapping: Ignores invalid yaml files.Jun 21 2022, 8:54 AM
VickyMerzOwn edited the summary of this revision. (Show Details)
VickyMerzOwn added a task: T4276: CffMapping: ignore invalid yaml files.
VickyMerzOwn updated this revision to Diff 28855.Jun 21 2022, 8:57 AM
Changes commit message
VickyMerzOwn added a comment.Jun 21 2022, 9:01 AM
Okay, got it. I have made these changes. Updated the diff title and is the summary of D8002 sufficient & correct now?



Also, I'll keep in mind the commit message imperative style along with diff update description styles.
swh-public-ci added a comment.Jun 21 2022, 9:03 AM
Build is green



Patch application report for D8002 (id=28855)



Rebasing onto 14f8a8574c...



First, rewinding head to replay your work on top of it...
Applying: cff: Ignore invalid yaml files





Changes applied before test



commit 359f1a8d9a0e3e70436465f3c3aa1cd2f91f735b
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    cff: Ignore invalid yaml files



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/269/ for more details.
Harbormaster completed remote builds in B29942: Diff 28855.Jun 21 2022, 9:03 AM
VickyMerzOwn marked 6 inline comments as done.Jun 21 2022, 10:42 AM
VickyMerzOwn updated this revision to Diff 28856.Jun 21 2022, 11:02 AM
Tests cff mappings of empty yaml files

Tests cff mappings of lists in yaml files
VickyMerzOwn updated this revision to Diff 28857.Jun 21 2022, 11:07 AM
Rebased
swh-public-ci added a comment.Jun 21 2022, 11:08 AM
Build is green



Patch application report for D8002 (id=28856)



Rebasing onto 14f8a8574c...



First, rewinding head to replay your work on top of it...
Applying: cff: Ignore invalid yaml files





Changes applied before test



commit 2a43d997b97f08a617723066e879752a8d80a2d7
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    cff: Ignore invalid yaml files



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/270/ for more details.
Harbormaster completed remote builds in B29943: Diff 28856.Jun 21 2022, 11:08 AM
swh-public-ci added a comment.Jun 21 2022, 11:13 AM
Build is green



Patch application report for D8002 (id=28857)



Rebasing onto 14f8a8574c...



Current branch diff-target is up to date.





Changes applied before test



commit 153bda12aa0d5cbcb55ac9b32539a79d2021ab60
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 17 14:14:32 2022 +0530

    cff: Ignore invalid yaml files



See https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/271/ for more details.
Harbormaster completed remote builds in B29944: Diff 28857.Jun 21 2022, 11:13 AM
vlorentz accepted this revision.Jun 21 2022, 11:25 AM
This revision is now accepted and ready to land.Jun 21 2022, 11:25 AM
Closed by commit rDCIDX153bda12aa0d: cff: Ignore invalid yaml files (authored by VickyMerzOwn).  ·  Explain WhyJun 21 2022, 11:25 AM
This revision was automatically updated to reflect the committed changes.
VickyMerzOwn added a commit: rDCIDX153bda12aa0d: cff: Ignore invalid yaml files.
Revision Contents
Changeset List
PathSize
swh/
indexer/
metadata_dictionary/
15 lines
tests/
53 lines
Diff 28858
View Options
swh/indexer/metadata_dictionary/cff.py
Loading...
View Options
swh/indexer/tests/test_metadata.py
Loading...
About · Developer information · Legal