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

Django 4.2/DjangoCMS 3.11 support #122

Open
11 tasks done
mogoh opened this issue Jun 26, 2023 · 25 comments
Open
11 tasks done

Django 4.2/DjangoCMS 3.11 support #122

mogoh opened this issue Jun 26, 2023 · 25 comments

Comments

@mogoh
Copy link
Contributor

mogoh commented Jun 26, 2023

If I ran python manage.py makemigrations after updating to Django 4.2/DjangoCMS 3.11, new migrations will be generated.

Housekeeping

Support of Versions:

py311-django42-cms311
py310-django42-cms311
py310-django32-cms{38, 39, 310}
py39-django32-cms{38, 39, 310}
py38-django32-cms{38, 39, 310}
  • Update setup.py for supported Versions
  • Add project.toml
  • Update CHANGELOG.rst
  • Add .pre-commit-config.yaml (Not sure how to use this)

Testing

  • Update tox.ini for testing. Update supported Versions
  • Autogenerate tests/requirements/* with compile.py

Pre commit linting

  • Update .github/workflows/lint.yml
    Change testing from flake8 & isort to ruff
  • Add .github/workflows/lint-pr.yml for conventional commits

Code Coverage

  • Add .github/workflows/test.yml.

Pypi publishing pipeline

  • Update .github/workflows/publish-to-test-pypi.yml
  • Update .github/workflows/publish-to-live-pypi.yml

Add migrations

There are already two pull requests, for adding migrations.

We should pick one, might need to rebase it onto master and merge it.
The other pull request should be closed.

@marksweb
Copy link
Member

Would be best with updates based on django-cms/djangocms-audio#32

@mogoh
Copy link
Contributor Author

mogoh commented Jul 10, 2023

I have edited the top comment into a to do list (more or less a list).

@mogoh
Copy link
Contributor Author

mogoh commented Jul 18, 2023

I have opened the pull request #123.
It is to fix the tasks of this issue and complement #120.

Question: How do I prepare the environment in a way, that I can run tests/requirements/compile.py?
Not every python-version is installed in my package manager.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 22, 2023

I managed to run compile.py with pyenv.
I think it should be considered to move a tool like compile.py into a separated tool repository.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 22, 2023

Can someone help me fixing the ci?
I tried to configure the .y(a)ml files but they are quit confusing and there is so much tooling involved.

@marksweb
Copy link
Member

I think it should be considered to move a tool like compile.py into a separated tool repository.

Because it's a script used to generate the test requirements and not a generic module I'm not sure it'd work as a separate tool. Although with the tweaks that @fsbraun made to it recently it could be made into something that could be packaged up.

@fsbraun
Copy link
Member

fsbraun commented Jul 22, 2023

@marksweb Maybe it's a candidate for a manual github action. Would require a runner with multiple python versions installed and prevent contributors from drowning their hard drives with countless python versions. This might not even be difficult: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#specifying-multiple-pythonpypy-version

  • Running the action would need to create a PR that updates the test requirements.
  • This would go through normal review.
  • We might need to think of a mechanism that once a PR is created just updates it to avoid having countless PRs on test requirements

@marksweb
Copy link
Member

@fsbraun yes, love this idea. That'd make it really easy to upgrade requirements as well.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 24, 2023

@marksweb @fsbraun

Although with the tweaks that @fsbraun made to it recently it could be made into something that could be packaged up.

What tweaks?
Where is the most recent version?
I did some tweaks myself: https://github.com/CERES-RUB/djangocms-picture/blob/feat-django-4-2-support/tests/requirements/compile.py

The problem with the same script in every repository, that they are drifting apart and it is hard to keep track of that.
It is for sure possible, that we turn this into a somewhat generic tool / module, that reeds the path of requirements.in from an environment variable or command line parameter.
But I understand, if that is not priority right now.

@marksweb
Copy link
Member

@mogoh I have tracked down the changes Fabian made to djangocms-alias:

https://github.com/django-cms/djangocms-alias/blob/master/tests/requirements/compile.py

Looks like you were thinking along the same lines with your changes.

If we can develop this into an action, then that solves the issue of the script drifting between repos.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 24, 2023

@marksweb
Ah, thanks!
I don't know about making it an action, because they are still complicated to me.
But if that solves the problem, I am fine with that.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 24, 2023

Here is another problem:

So we are both depending on each other.
Proposed solutions:

  • Create a feature branch, merge both into the feature branch and then merge that feature branch.
  • Integrate one pull request into the other and delete the original pull request. Then merge both.

@vasekch
Copy link
Contributor

vasekch commented Jul 25, 2023

* Integrate one pull request into the other and delete the original pull request. Then merge both.

I'm happy with closing mine PR in favor of #123, mine is one independent file so it will be easy to integrate.

@mogoh feel free to close #120 when done.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 26, 2023

I'm happy with closing mine PR in favor of #123, mine is one independent file so it will be easy to integrate.

@vasekch thx, I will integrate your file. But I am no maintainer, so someone else must close your pull request, once it is done.

@mogoh
Copy link
Contributor Author

mogoh commented Jul 28, 2023

If I run

tox run -f py38
tox run -f py311
python -m coverage report --fail-under=100

I get a coverage report of 100%.

If I how ever run

tox run -f py311
tox run -f py38
python -m coverage report --fail-under=100

I get a coverage report of 99%.

This suggest to me, that our coverage report is misconfigured and only works for the latest run of tox.

I think the reason, why coverage varies between python version might be the coverage of decorator functions.
Also, if coverage vary between python versions, it might be this reason: nedbat/coveragepy#866

I don't know how to achieve 100% coverage and help would be appreciated.

@marksweb
Copy link
Member

@mogoh Yeah I've never really liked configuring coverage and often found similar results.

There is the combine command (docs). This combines multiple data files, so the idea being you run this before running the report.

So what happens if you run;

tox run -f py311
tox run -f py38

python -m coverage combine
python -m coverage html --skip-covered --skip-empty
python -m coverage report --fail-under=100

@mogoh
Copy link
Contributor Author

mogoh commented Jul 31, 2023

So, I learned that to combine, one needs to run coverage run --parallel-mode, so that the .coverage files are created like .coverage.<something>.
So I did that.

But the problem did not went away.
I have no idea why.

Here is what I did:

coverage erase

tox run -f py311
tox run -f py38

coverage combine
coverage html
coverage report --fail-under=100

This leads to 100% coverage

However this does not:

coverage erase

tox run -f py38

coverage combine
coverage html
coverage report --fail-under=100

Only 99% coverage.

@mogoh
Copy link
Contributor Author

mogoh commented Aug 2, 2023

If no one can find a solution, I suggest removing --fail-under=100 and accept less then 100% coverage.

@fsbraun
Copy link
Member

fsbraun commented Aug 2, 2023

Did you look into the different coverage reports. I would assume that some code is only run by py11...?

@mogoh
Copy link
Contributor Author

mogoh commented Aug 2, 2023

Here are some reports:

coverage erase
tox run -f py311
coverage combine
coverage html

yields to

image

coverage erase
tox run -f py38
coverage combine
coverage html

yields to

image

image

(BTW: coverage doesn't generate plain text reports, only short summaries, right? Thus screenshotted html reports.)

@fsbraun
Copy link
Member

fsbraun commented Aug 2, 2023

Hm, that seems more like differences between python versions counting lines..., Since it works with the newer versions, I'd leave the target. We all know that the 99% are ok.

@mogoh
Copy link
Contributor Author

mogoh commented Aug 10, 2023

Sorry, for taking so long to answer.

@fsbraun I am not sure if I understand you correctly.

If we don't change the 100%-code-coverage-target, we are not able to merge the change into master branch, right?

Since it works with the newer versions, I'd leave the target.

What do you mean by that? Leave the target in place (do not change it)? Or leave the target behind and change it?

Because the way I understand you is, that we should change it, but then the rest does not fit.
Please clarify.

@marksweb
Copy link
Member

@mogoh if the 100% thing is the problem, just remove that from the report command.

@mogoh
Copy link
Contributor Author

mogoh commented Aug 11, 2023

Should this issue stay open, until the new support is released or should we close this?
The other pull-requests (#118 and #120) should be closed in any case.

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

No branches or pull requests

4 participants