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

Refresh and automate updating licenses list #489

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Refresh and automate updating licenses list #489

merged 2 commits into from
Aug 10, 2023

Conversation

Secrus
Copy link
Member

@Secrus Secrus commented Oct 3, 2022 •
edited
Loading

Resolves: python-poetry/poetry#6689

  • Added tests for changed code. (no significant changes)
  • Updated documentation for changed code. (internal changes)

colindean
colindean previously approved these changes Oct 4, 2022
View reviewed changes
Copy link

@colindean colindean left a comment

Choose a reason for hiding this comment

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

LGTM, just about picked this up myself as a Hacktoberfest project

@colindean colindean mentioned this pull request Oct 4, 2022
Closed
4 tasks
@neersighted
Copy link
Member

@Secrus would you mind dropping the update, and adding a workflow_dispatch event so we can manually trigger your workflow? I'd rather do that so we can test the workflow now and fix any issues.

neersighted
neersighted previously requested changes Oct 10, 2022
View reviewed changes
src/poetry/core/spdx/license.py Outdated Show resolved Hide resolved
src/poetry/core/spdx/license.py Outdated Show resolved Hide resolved
src/poetry/core/spdx/license.py Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Oct 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus
Copy link
Member Author

Secrus commented Oct 11, 2022

@neersighted I have added the event to the workflow and reverted changes (don't worry about the commit history, I will clean it up before merging)

mkniewallner
mkniewallner reviewed Oct 11, 2022
View reviewed changes
Copy link
Member

@mkniewallner mkniewallner 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.

I'm wondering if there's a valid reason for having the updater in poetry-core itself. Wouldn't it be more suitable to have it in a dedicated scripts directory outside the package?

.github/workflows/update_licenses.yml Outdated Show resolved Hide resolved
@mkniewallner mkniewallner mentioned this pull request Oct 29, 2022
Closed
2 tasks
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from 7b2a532 to dbaf122 Compare November 28, 2022 03:24
@sonarcloud
Copy link

sonarcloud bot commented Nov 28, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus
Copy link
Member Author

Secrus commented Nov 28, 2022

@neersighted @mkniewallner I reduced this PR to just a workflow, I will be refactoring licensing in another PR.

@Secrus Secrus requested review from neersighted, colindean and mkniewallner and removed request for colindean November 28, 2022 03:26
colindean
colindean previously approved these changes Nov 30, 2022
View reviewed changes
.github/workflows/update_licenses.yml Outdated Show resolved Hide resolved
eamanu
eamanu previously approved these changes May 13, 2023
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

Hi, this PR LGTM. It would great have the license.py updated. Any possibility to merge it?

@eamanu
Copy link
Contributor

eamanu commented May 22, 2023

Hi, @Secrus Are you still interest to continue this PR? :-)

@Secrus
Copy link
Member Author

Secrus commented May 22, 2023

@eamanu Yeah, I will try to merge it this week

@Secrus Secrus dismissed stale reviews from eamanu and colindean via 4ad85d2 May 23, 2023 22:21
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from dbaf122 to 4ad85d2 Compare May 23, 2023 22:21
@Secrus Secrus requested a review from a team May 23, 2023 22:23
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from 4ad85d2 to f6a3c4d Compare June 2, 2023 23:12
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

radoering
radoering reviewed Jun 3, 2023
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Since no member with more experience in github actions seems to be available, I'll do my best and probably ask some stupid questions. 😉

.github/workflows/update_licenses.yml Outdated Show resolved Hide resolved
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch 2 times, most recently from 839d37d to be1424d Compare July 15, 2023 12:46
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from be1424d to 96c7f6f Compare July 15, 2023 12:49
@Secrus Secrus requested a review from radoering July 20, 2023 15:43
@Secrus Secrus mentioned this pull request Jul 28, 2023
Closed
radoering
radoering reviewed Jul 30, 2023
View reviewed changes
.github/workflows/update_licenses.yml Outdated Show resolved Hide resolved
.github/workflows/update_licenses.yml Outdated Show resolved Hide resolved
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from 96c7f6f to 6b006ee Compare July 30, 2023 23:13
@Secrus Secrus force-pushed the refresh-and-automate-licenses branch from 6b006ee to 38394b2 Compare July 30, 2023 23:14
@radoering
Copy link
Member

LGTM to me now. Can we test it before merging as proposed in #489 (comment)?

radoering
radoering approved these changes Aug 10, 2023
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Probably too much effort to test before merging so let's try.

@sonarcloud
Copy link

sonarcloud bot commented Aug 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Secrus Secrus removed the request for review from neersighted August 10, 2023 19:35
@Secrus Secrus merged commit e02ee6b into python-poetry:main Aug 10, 2023
19 checks passed
@dimbleby
Copy link
Contributor

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

@Secrus
Copy link
Member Author

Secrus commented Aug 10, 2023

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

of course it will, (and actually did), but we accepted that risk. See #622 for fixes

@dimbleby
Copy link
Contributor

are you in a suitable virtual environment when you call the script? won't it just fail with 'No module named poetry'?

ah never mind, #622, I am too slow

@Secrus Secrus deleted the refresh-and-automate-licenses branch August 11, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgarrmondragon edgarrmondragon edgarrmondragon left review comments

@neersighted neersighted neersighted left review comments

@radoering radoering radoering approved these changes

@eamanu eamanu eamanu left review comments

@colindean colindean colindean left review comments

@mkniewallner mkniewallner Awaiting requested review from mkniewallner

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

Successfully merging this pull request may close these issues.

The Unlicense not recognized as a license
8 participants
@Secrus @neersighted @eamanu @radoering @dimbleby @colindean @mkniewallner @edgarrmondragon