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

Add info field to RepositoryVersion #2999

Merged
merged 1 commit into from
Aug 10, 2022

Conversation

hstct
Copy link
Contributor

@hstct hstct commented Jul 22, 2022

closes #2998

The name of the field was chosen arbitrarily and is certainly open to be changed into whatever fits the best.

pulpcore/app/models/repository.py Outdated Show resolved Hide resolved
pulpcore/app/serializers/repository.py Outdated Show resolved Hide resolved
hstct added a commit to ATIX-AG/pulp_deb that referenced this pull request Jul 25, 2022
quba42 pushed a commit to ATIX-AG/pulp_deb that referenced this pull request Aug 2, 2022
Required PR: pulp/pulpcore#2999

closes pulp#564

Co-Authored-By: Quirin Pamp <pamp@atix.de>

This will change the default behaviour of the sync to use optimize
unless it is explicitly set to False.
@quba42
Copy link
Contributor

quba42 commented Aug 2, 2022

We now have a working state of the pulp_deb optmize sync that needs this field.

As such, it would be nice if we could settle on a name for the field, so we can update this PR and move towards final review.

sync_details is not a good name, since I definitely envision the pulp_deb advanced copy API also making use of this field in the future (that is it is not just intended for syncs).

Personally I vote for creation_details, because this is somewhat opinionated about what this field is intended for. Given that the field can factually contain arbitrary json data, a completely generic name like version_details would alternatively also make sense.

Does anyone want to wade in, or should we go ahead and change this PR to use creation_details?

@dkliban
Copy link
Member

dkliban commented Aug 2, 2022

What about just 'details' or 'info' ?

@ggainey
Copy link
Contributor

ggainey commented Aug 2, 2022

+1 to "info"

@quba42
Copy link
Contributor

quba42 commented Aug 2, 2022

The current consensus is "info".

@hstct hstct force-pushed the additional-repoversion-field branch from 4cebd76 to 42ce95d Compare August 2, 2022 15:14
@hstct hstct changed the title Add sync_details field to RepositoryVersion Add info field to RepositoryVersion Aug 2, 2022
@hstct
Copy link
Contributor Author

hstct commented Aug 2, 2022

Updated the PR renamed the field to info and removed exposing the data in the serializer.

CHANGES/2998.feature Outdated Show resolved Hide resolved
@hstct hstct force-pushed the additional-repoversion-field branch from 42ce95d to 7763977 Compare August 3, 2022 08:25
hstct added a commit to ATIX-AG/pulp_deb that referenced this pull request Aug 3, 2022
Required PR: pulp/pulpcore#2999

closes pulp#564

Co-Authored-By: Quirin Pamp <pamp@atix.de>

This will change the default behaviour of the sync to use optimize
unless it is explicitly set to False.
@ipanova
Copy link
Member

ipanova commented Aug 3, 2022

The current consensus is "info".

Sorry for back and forth on this, I would personally vote for more explicit name like version_details or was the intention to leave this as generic as possible to leave the door opened to add such field to other models too? @ggainey @dkliban

@quba42
Copy link
Contributor

quba42 commented Aug 3, 2022

@ipanova I guess one could consider the version_ part in RepositoryVersion.version_<whatever> redundant?
If so, then info or details seems just as good as version_info or version_details. (I have no opinion on info vs details).

The other thought was that since the field is inherently generic (can contain arbitrary Json), the name should also be generic?
My original thought was that the field name could be more "opinionated" than the field itself, which is why I suggested creation_details, but this was never a strong opinion. At the end of the day I am fine with any of the suggestions within this thread (so long as they don't explicitly include sync in the name).

My 2ct.

Copy link
Member

@ipanova ipanova left a comment

Choose a reason for hiding this comment

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

@dralley if you want to also take a look

@dralley
Copy link
Contributor

dralley commented Aug 9, 2022

If we're going for a generic name like "info" then we maybe ought to consider whether this could be more broadly applicable than just repository versions, something like the Pulp 2 "scratchpad". For example I can see a use case having publications store particular details about how they were created as well.

But on the flip side, I'm not sure if we want to resurrect the "scratchpad", and we already have a label system which has a small overlap in intended purpose.

@ipanova
Copy link
Member

ipanova commented Aug 9, 2022

If we're going for a generic name like "info" then we maybe ought to consider whether this could be more broadly applicable than just repository versions, something like the Pulp 2 "scratchpad". For example I can see a use case having publications store particular details about how they were created as well.

But on the flip side, I'm not sure if we want to resurrect the "scratchpad", and we already have a label system which has a small overlap in intended purpose.

If my recollection is correct, I believe we discussed this aspect in pulpcore meeting, that's why we're trying to pick as generic name as we can.
I agree with you about the feeling of adding this to all models, hence I'd say let's add only where we have a request for now?

Labels are user visible, this is not intended to be seen.

@dralley
Copy link
Contributor

dralley commented Aug 9, 2022

Unfortunately once it's been added to a model it can't just be easily moved to another model in the inheritance hierarchy. You can make a new field with a different name but keeping the same name is really problematic.

@ipanova
Copy link
Member

ipanova commented Aug 9, 2022

Unfortunately once it's been added to a model it can't just be easily moved to another model in the inheritance hierarchy. You can make a new field with a different name but keeping the same name is really problematic.

oh, i see, yes

Copy link
Contributor

@dralley dralley left a comment

Choose a reason for hiding this comment

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

If we were ever to remove this and move it to the base model we would likely call it pulp_info, precluding the aforementioned concern.

@ipanova ipanova merged commit 7eab840 into pulp:main Aug 10, 2022
@quba42 quba42 deleted the additional-repoversion-field branch August 10, 2022 11:20
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 another field to the RepositoryVersion model to store additional information.
8 participants