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.

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

SPDX License feature UI and task changes #2296

Merged
merged 6 commits into from
Nov 25, 2021

Conversation

msys-sgarg
Copy link
Contributor

@msys-sgarg msys-sgarg commented Oct 14, 2021

Description

With this change
if any cook has a license mentioned and there is a respective entry for license url in https://github.com/spdx/license-list-data/blob/master/json/licenses.json

The license url information will be updated for the cookbook and a link will be shown in ui for the user to go through the license url

Have added ctl commands for updating license information in supermarket deployments. Attaching some screenshots for reference
After running ctl command or uploading a new cookbook UI changes :
Screenshot from 2021-11-11 22-42-43

CTL command running succesfully
Screenshot from 2021-11-12 01-44-27

Issues Resolved

#2188

Check List

@msys-sgarg msys-sgarg requested review from a team as code owners October 14, 2021 10:55
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 4 times, most recently from 292215b to fe463f1 Compare October 14, 2021 12:41
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch from 08caf4a to 6c16204 Compare October 18, 2021 05:43
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 2 times, most recently from bea5c81 to daeca94 Compare October 18, 2021 07:10
[:error, e]
end

end
Copy link
Contributor

Choose a reason for hiding this comment

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

as per the lint pattern we should add a newline at the end of every file.

Suggested change
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But linter never complained about this .. if this would have been the rule .. linter would have failed ? I don't suggest making a change without linter complaining about it explicitly

Copy link
Contributor

Choose a reason for hiding this comment

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

If you see the file in github diff you will notice. Sharing SS
Screenshot 2021-10-18 at 1 00 13 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it's a guideline from rubocop. Refer this link:
https://www.rubydoc.info/gems/rubocop/0.20.1/Rubocop/Cop/Style/FinalNewline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be a guideline but in our project we are not following that .. no project follows all the guidelines from rubocop.. I will make a change thats not an issue .. but I expect linter to suggest linter changes if this has to be followed .. this has to be put in the cops definition for the project

Copy link
Contributor

Choose a reason for hiding this comment

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

yes makes sense.


[:ok, I18n.t("spdx_license.scheduled.single", name: cookbook.name, version: cookbook_version.id)]
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

as per the lint pattern we should add a newline at the end of every file.

Suggested change
end
end

src/supermarket/app/workers/spdx_license_update_worker.rb Outdated Show resolved Hide resolved
src/supermarket/app/workers/spdx_license_update_worker.rb Outdated Show resolved Hide resolved
src/supermarket/db/schema.rb Show resolved Hide resolved
exit 1
end

result, message = UpdateSpdxLicenseUrl.on_version args[:cookbook_name], args[:version]
Copy link
Contributor

Choose a reason for hiding this comment

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

when u use multiple assignment use parenthesis to mark the scope of function arguments for better readibility.

Suggested change
result, message = UpdateSpdxLicenseUrl.on_version args[:cookbook_name], args[:version]
result, message = UpdateSpdxLicenseUrl.on_version(args[:cookbook_name], args[:version])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of language like ruby I do not agree with this suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

No issues from the language point of view. It's just for readibility. U can keep it as it is.

@msys-sgarg msys-sgarg changed the title Changes for update spdx worker SPDX License feature UI and task changes Oct 18, 2021
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 2 times, most recently from 22529d4 to c6ffaa9 Compare October 19, 2021 13:20
@saghoshprogress
Copy link
Contributor

@RajeshPaul38 Please merge this if everything looks good.

@RajeshPaul38
Copy link
Contributor

@RajeshPaul38 Please merge this if everything looks good.

Have approved. Pls go ahead.

@@ -25,3 +25,4 @@ FIERI_FOODCRITIC_TAGS="metadata,correctness,supermarket,portability,deprecated ~
FIERI_SUPERMARKET_ENDPOINT=http://localhost:13000
ROBOTS_ALLOW=/
ENFORCE_PRIVACY=true
SPDX_LICENSE_URL=https://raw.githubusercontent.com/spdx/license-list-data/master/json/licenses.json
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make sure that Supermarket can work with as little external connectivity as possible. Is it possible to fetch this file in the omnibus build so we ship Supermarket with the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will make this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, How often do we expect this file to get updated? What will happen if the file gets updated since we have static copy of the same now?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be pulling in the latest copy at build time in Supermarket.

@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 5 times, most recently from f1683b9 to 6b9e714 Compare November 8, 2021 10:08
@msys-sgarg msys-sgarg changed the title SPDX License feature UI and task changes WIP:SPDX License feature UI and task changes Nov 9, 2021
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 4 times, most recently from 98e0d50 to 7c64054 Compare November 10, 2021 14:28
@msys-sgarg msys-sgarg changed the title WIP:SPDX License feature UI and task changes SPDX License feature UI and task changes Nov 11, 2021
@msys-sgarg msys-sgarg changed the title SPDX License feature UI and task changes WIP:SPDX License feature UI and task changes Nov 11, 2021
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch from 2ce2213 to 795cad3 Compare November 11, 2021 11:05
@msys-sgarg msys-sgarg requested a review from a team as a code owner November 12, 2021 04:30
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch from 50232c9 to 70e4aa8 Compare November 12, 2021 08:21
@msys-sgarg msys-sgarg changed the title WIP:SPDX License feature UI and task changes SPDX License feature UI and task changes Nov 12, 2021
@msys-sgarg
Copy link
Contributor Author

Signed-off-by: smriti <sgarg@msystechnologies.com>
Signed-off-by: smriti <sgarg@msystechnologies.com>
Signed-off-by: smriti <sgarg@msystechnologies.com>
Signed-off-by: smriti <sgarg@msystechnologies.com>
Signed-off-by: smriti <sgarg@msystechnologies.com>
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch from 865a0a1 to 397cf27 Compare November 24, 2021 13:00
@netlify
Copy link

netlify bot commented Nov 24, 2021

👷 Deploy Preview for chef-supermarket processing.

🔨 Explore the source changes: fc2fcee

🔍 Inspect the deploy log: https://app.netlify.com/sites/chef-supermarket/deploys/619e39f6289a4c000857151f

@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch 2 times, most recently from ceb1993 to b782667 Compare November 24, 2021 13:03
Signed-off-by: smriti <sgarg@msystechnologies.com>
@msys-sgarg msys-sgarg force-pushed the smriti/2188_spdx_license_link_to_cookbook branch from b782667 to fc2fcee Compare November 24, 2021 13:11
@github-actions
Copy link

Simplecov Report

Covered Threshold
97.24% 90%

@tas50 tas50 merged commit 6f9b8d2 into main Nov 25, 2021
@tas50 tas50 deleted the smriti/2188_spdx_license_link_to_cookbook branch November 25, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants