Skip to content

Comments

Refresh and automate updating licenses list#489

Merged
Secrus merged 2 commits intopython-poetry:mainfrom
Secrus:refresh-and-automate-licenses
Aug 10, 2023
Merged

Refresh and automate updating licenses list#489
Secrus merged 2 commits intopython-poetry:mainfrom
Secrus:refresh-and-automate-licenses

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

neersighted commented Oct 10, 2022

@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
@sonarqubecloud
Copy link

sonarqubecloud 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?

@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
@sonarqubecloud
Copy link

sonarqubecloud 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 colindean, mkniewallner and neersighted and removed request for colindean November 28, 2022 03:26
colindean
colindean previously approved these changes Nov 30, 2022
View reviewed changes
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
@sonarqubecloud
Copy link

sonarqubecloud 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. 😉

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

radoering commented Jul 31, 2023

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.

@sonarqubecloud
Copy link

sonarqubecloud 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 dismissed neersighted’s stale review August 10, 2023 19:36

no longer valid

@Secrus Secrus merged commit e02ee6b into python-poetry:main Aug 10, 2023
@dimbleby
Copy link
Contributor

dimbleby 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'?

@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

dimbleby 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'?

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

@neersighted neersighted neersighted left review comments

@radoering radoering radoering approved these changes

@mkniewallner mkniewallner Awaiting requested review from mkniewallner

+3 more reviewers

@edgarrmondragon edgarrmondragon edgarrmondragon left review comments

@colindean colindean colindean left review comments

@eamanu eamanu eamanu left review comments

Reviewers whose approvals may not affect merge requirements

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