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.

Sign up for GitHub

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

Jump to bottom

Add Conda Support #1019

Merged
merged 20 commits into from
Mar 19, 2024
Merged

Add Conda Support #1019

merged 20 commits into from
Mar 19, 2024

Conversation

lamarrr
Copy link
Contributor

@lamarrr lamarrr commented Nov 20, 2023

Service Implementation for clearlydefined/crawler#532

@lamarrr lamarrr force-pushed the conda-support branch 4 times, most recently from 88cf587 to a290ed0 Compare November 24, 2023 13:09
@lamarrr lamarrr marked this pull request as ready for review November 24, 2023 13:14
conda service implementation
c2fc97c 
removed extra semicolon

added architecture to conda revisions

removed base version

fix version fetching

fixed lint errors

update

added tests

Fix license not detected for some maven packages

When processing scancode result, license information is first derived from package level data.  When this is not available, ScanCodeSummarizer goes through the root files to look for license.  Filtering for root files is based on case sensitive file path matching.  In maven packaging, META/INF directory is the standard (see https://issues.apache.org/jira/browse/MJAR-73?attachmentOrder=desc, and https://issues.apache.org/jira/browse/MEAR-30).  Change the location for maven to reflect the casing.

utils.getLicenseLocations is also used in utils.isLicenseFile.  isLicenseFile uses case insensitive matching.  Preserve this case insensitive file path matching by converting the result from getLicenseLocations to lowercase there.

Test case:
https://clearlydefined.io/definitions/maven/mavencentral/org.flywaydb/flyway-core/9.20.0
https://dev.clearlydefined.io/definitions/maven/mavencentral/org.flywaydb/flyway-core/7.7.2

Task: clearlydefined#846

Fix license detection for some sourcearchive packages

Task: clearlydefined/crawler#533

fixed test

update
qtomlinson
qtomlinson reviewed Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@qtomlinson qtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
Adaptation also needed: schema/coordinates-1.0.json, swagger.yaml (probably add on subdir mapped to namespace for conda in the description), and docs/determining-declared-license.md.

providers/summary/clearlydefined.js Outdated Show resolved Hide resolved
routes/originConda.js Outdated Show resolved Hide resolved
routes/originConda.js Outdated Show resolved Hide resolved
routes/originConda.js Outdated Show resolved Hide resolved
qtomlinson
qtomlinson requested changes Feb 9, 2024
View reviewed changes
Copy link
Collaborator

@qtomlinson qtomlinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for making the changes. As per previous comments, please also adapt schema/coordinates-1.0.json, swagger.yaml (include subdir mapped to namespace for conda in the description for clarity), and docs/determining-declared-license.md.

routes/originConda.js Outdated Show resolved Hide resolved
@lamarrr
Copy link
Contributor Author

lamarrr commented Feb 9, 2024

Thanks for making the changes. As per previous comments, please also adapt schema/coordinates-1.0.json, swagger.yaml (include subdir mapped to namespace for conda in the description for clarity), and docs/determining-declared-license.md.

all done!

qtomlinson
qtomlinson reviewed Feb 10, 2024
View reviewed changes
Copy link
Collaborator

@qtomlinson qtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

docs/determining-declared-license.md Outdated Show resolved Hide resolved
routes/originConda.js Show resolved Hide resolved
schemas/swagger.yaml Outdated Show resolved Hide resolved
qtomlinson
qtomlinson reviewed Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@qtomlinson qtomlinson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes look good to me. One question, for the cases of 404, [ ] is sent on line 43, while error messages are sent on line 47 and 50. Is it better to make it more consistent for all the 404 cases so that it is easier to handle on the caller side?

@qtomlinson qtomlinson self-requested a review February 14, 2024 22:28
qtomlinson

This comment was marked as duplicate.

Sign in to view

@lamarrr
Copy link
Contributor Author

lamarrr commented Feb 14, 2024

Changes look good to me. One question, for the cases of 404, [ ] is sent on line 43, while error messages are sent on line 47 and 50. Is it better to make it more consistent for all the 404 cases so that it is easier to handle on the caller side?

Yeah, that makes sense, I'll do that tomorrow morning. thanks for your review!

lamarrr and others added 2 commits March 1, 2024 23:29
@lamarrr @lisahoong
Update schemas/definition-1.0.json
24c49e2 
Co-authored-by: lisahoong <lhoong94@gmail.com>
@lamarrr
Update definition-1.0.json
3b8b417
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 15, 2024
View reviewed changes
routes/originConda.js Fixed Show fixed Hide fixed
routes/originConda.js Fixed Show fixed Hide fixed
routes/originConda.js Fixed Show fixed Hide fixed
routes/originConda.js Fixed Show fixed Hide fixed
routes/originConda.js Fixed Show fixed Hide fixed
@lumaxis
Copy link
Contributor

lumaxis commented Mar 19, 2024

@lamarrr Could you address these code scanning issues? Once these are fixed, we can finally merge this 👍🏼

@lamarrr
Copy link
Contributor Author

lamarrr commented Mar 19, 2024

@lumaxis they are fixed now

@lumaxis lumaxis merged commit ca82f80 into clearlydefined:master Mar 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lisahoong lisahoong lisahoong left review comments

@qtomlinson qtomlinson qtomlinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lamarrr @lumaxis @lisahoong @qtomlinson