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

Fix #2341: Replace liqd/django-autoslug dependency to upstream #2446

Closed
wants to merge 1 commit into from

Conversation

DubeySandeep
Copy link

Overview

This PR fixes #2341 by replacing the forked/edited django-autoslug (introduced via #2188) with its original upstream.

This is possible now as the changes made on the forked repo are merged and released upstream.

Manual testing

Able to install the latest upstream django-autoslug via pip
image

Able to run the server with the latest version of django-autoslug
image

@@ -16,7 +16,7 @@ urllib3==1.26.15
# Inherited a4-core requirements
Django==3.2.19
django-allauth==0.54.0
git+https://github.com/liqd/django-autoslug.git@liqd2212#egg=django-autoslug
Copy link
Author

Choose a reason for hiding this comment

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

Looks like a4-core also uses this dep., I'll initiate a PR there as well so that we can archive/delete (if needed) liqd/django-autoslug once bother the PRs are merged.

Copy link
Author

Choose a reason for hiding this comment

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

Done! (Initiated a PR liqd/adhocracy4#1413)

@goapunk
Copy link
Contributor

goapunk commented May 31, 2023

Hey @DubeySandeep, thanks for your PRs and contributions! We are currently discussing internally whether we should have a CLA for external contributors. We probably shouldn't have one as it makes contributing harder but as this is a team decision it will still take us some time and I would like to wait until then before we merge this, sorry!

@goapunk goapunk self-requested a review May 31, 2023 07:40
@DubeySandeep
Copy link
Author

@goapunk Thanks for the review and the heads-up! I think it's a good idea to have a CLA (assuming t will be a simple form (max 3 inputs)), I'm fine with signing the CLA, let me know once you have an update! :)

Also, it looks like we are using a secret (secret.COV) which is not permissible to forked PRs build (ref), resulting into empty string value for the token and blocking my PR build to complete the Coveralls check.

Possible solutions:

  • Use secrets.GITHUB_TOKEN similar to line 92 (Note: I don't have context for the usage of COV secret and whether it can be replaced by secrets.GITHUB_TOKEN)
  • Use pull_request_target event as suggested in the doc.

@DubeySandeep
Copy link
Author

Hi @goapunk, I wanted to check whether you have any updates on the CLA requirements.

@goapunk
Copy link
Contributor

goapunk commented Jun 20, 2023

Hi @DubeySandeep, sorry for the late reply and thanks again for your contribution. We should have something ready by the end of the week. We'll also look into fixing the CI/Coveralls for external PRs, thanks for reporting the issue and the suggestions!

@goapunk
Copy link
Contributor

goapunk commented Jun 17, 2024

superseded by #2710

@goapunk goapunk closed this Jun 17, 2024
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.

move back to upstream django-autoslug
2 participants