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

Updated deps accoring to the docs guidelines. #1094

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

ipanova
Copy link
Member

@ipanova ipanova commented Oct 3, 2022

closes #1093

@ipanova ipanova force-pushed the declare-deps branch 2 times, most recently from 66a62a2 to 6f99d06 Compare October 3, 2022 11:48
@ipanova
Copy link
Member Author

ipanova commented Oct 3, 2022

@bmbouter is this what you've expected from a plugin adhering to the new guidelines?

requirements.txt Outdated
django-filter>=22.1,<=22.1
django-import-export>=2.9,<=2.9.0
drf-access-policy>=1.1.2,<=1.1.2
drf-spectacular==0.24.2 # We monkeypatch this so we need a very narrow requirement string
Copy link
Member

Choose a reason for hiding this comment

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

Do you directly import this?

Anything pulp_container doesn't directly import doesn't need to be declared. Perhaps you do directly import this though....

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we do

[ipanova@puffy pulp_container]$ git grep spectac
.ci/scripts/schema.py:This script modifies drf-spectacular schema validation to accept slashes and curly brackets.
.ci/scripts/schema.py:from drf_spectacular.validation import JSON_SCHEMA_SPEC_PATH
pulp_container/app/viewsets.py:from drf_spectacular.utils import extend_schema
[ipanova@puffy pulp_container]$ git grep extend_schema
pulp_container/app/viewsets.py:from drf_spectacular.utils import extend_schema
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
pulp_container/app/viewsets.py:    @extend_schema(
[ipanova@puffy pulp_container]$ 

Copy link
Member

Choose a reason for hiding this comment

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

I would expect us to have a very broad range here. Pinning to the same version as pulpcore is calling for desaster. Also i think the monkey-patching does not apply here.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I think this can be very broad and it needs to be or this strategy won't work well.

Copy link
Member

Choose a reason for hiding this comment

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

As an aside, we need pulpcore to stop pinning.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but I'm wondering what we can do to stop monkey patching. (apologies @ipanova for hijacking this PR for a pulpcore discussion). @mdellweg we probably need a pulpcore issue to discuss on.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing without badly breaking the client bindings.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bmbouter @mdellweg what range do you expect to be used here?

Copy link
Member

Choose a reason for hiding this comment

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

I would still call drf-spectacular part of the pulpcore plugin interface. But I do not intend to reroll the whole discussion from the beginning.
How about drf-spectacular>=0.24,<0.25?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with either drf-spectacular>=0.24,<0.25 or not declaring it at all.

@bmbouter
Copy link
Member

bmbouter commented Oct 3, 2022

@bmbouter is this what you've expected from a plugin adhering to the new guidelines?

I left a question, but style-wise yes this is exactly what I was expecting.

@ipanova
Copy link
Member Author

ipanova commented Oct 3, 2022

@bmbouter is this what you've expected from a plugin adhering to the new guidelines?

I left a question, but style-wise yes this is exactly what I was expecting.

ty for proofreading

Comment on lines +1 to +9
version: 2
updates:
- package-ecosystem: pip
directory: "/"
schedule:
interval: weekly
open-pull-requests-limit: 10
commit-message:
prefix: "[noissue]"
Copy link
Member

Choose a reason for hiding this comment

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

Ugh this is what will activate Dependabot on all forks.

Any new reason to add this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes? who will manage the automatic updates of the released deps?https://github.com/pulp/pulpcore/blob/main/docs/plugins/plugin-writer/concepts/index.rst#declaring-dependencies

I don;t think it will activate on forks, can you point me to the config you then expect to see?

Copy link
Member

Choose a reason for hiding this comment

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

This is rather a big annoyance. Not a reason to stop doing this.

Copy link
Member

Choose a reason for hiding this comment

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

@ipanova ipanova marked this pull request as draft October 26, 2022 09:24
@ipanova ipanova marked this pull request as ready for review November 23, 2022 17:03
@ipanova ipanova merged commit bbb50a0 into pulp:main Nov 23, 2022
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.

Add upper and lower bounds to all pulp_container dependencies #3232
3 participants