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

build(deps): bump djangorestframework #1093

Closed
wants to merge 1 commit into from
Closed

build(deps): bump djangorestframework #1093

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ dependencies = [
"django-settings-export~=1.2",
"django-split-settings~=1.2",
"django-widget-tweaks~=1.5",
"djangorestframework~=3.14",
"djangorestframework~=3.15",
Copy link
Member

@jochenklar jochenklar Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No the dependency needs to be ~=3.15.2. I found the problem in this commit: f1098eb. I don't know if the changes where done automatic or by hand, but they need to be reversed and if they were done by the CI we need to be sure that this does not happen automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand. Why to you want to reverse a commit from 09/23?

Didn't we discuss this and decided to pin versions as MAJOR.MINOR, if they don't have a major version of zero?

Please feel free to pin everything with exact M.M.P versions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but this is not pinning as MAJOR.MINOR, if you use the ~= and give the full version MAJOR.MINOR.PATCH, than its pinned as MAJOR.MINOR. With only MAJOR.MINOR its pinned as MAJOR.

Copy link
Member Author

@afuetterer afuetterer Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Frankly I still understand the problem. Yes, I used the word pinning here, which is inaccurate. But the dependencies are declared like this, M.M form, for almost a year now.

We discussed this and I did not make that decision on my own.

See e.g. #693 (comment)

I even added a note right above the dependencies for this:

rdmo/pyproject.toml

Lines 40 to 42 in 065d195

# dependencies with major version on zero are declared with
# major.minor.patch, because they can potentially introduce breaking changes
# in minor version updates anytime

But of course I made a mistake, by not checking when the DRF warning was introduced. I just wanted to reduce the noise in the test run.

Please by any means feel free to pin however you like.

You can pin like this ==3.15.2 if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, thanks for looking it up. Sorry, I now remember that we discussed this back then and converged to this solution. I think we need to change this now, since we are now in the situation that we need to specify a minimum version. Maybe we need to go to the regular >=MAJOR.MINOR.PATCH with an optional <MAJOR.MINOR.PATCH, which should also match better with what dependabot does.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind about the exact notation but would prefer to have it picked up by the dependabot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I repeat: Feel free to pin however you like.

In my opinion ~=3.15, as proposed in this PR, would have fixed the issue I introduced, where I just wanted to help out.

I will not touch the dependencies again.

Copy link
Member

Choose a reason for hiding this comment

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

Hi and thanks for your help again. Right now, there is to much going on to comment on every PR timely, sorry. The pinning of the dependencies need to changed, but @MyPyDavid and I will work on that, probably next week. The problem is that your change will not fix the problem, since it will not install 3.15 when 3.14 is already installed. (or I got something wrong).

"drf-extensions~=0.7.1",
"iso8601~=2.0",
"markdown~=3.4",
Expand Down
Loading