Skip to content

Conversation

@rohanpm
Copy link
Contributor

@rohanpm rohanpm commented Aug 13, 2019

This attribute tells us whether a repository should be considered
"temporary" (i.e. it's created to support execution of some
workflow and should be deleted once that's complete).

@rohanpm
Copy link
Contributor Author

rohanpm commented Aug 13, 2019

@rajulkumar this is in response to the autotest you added here: https://github.com/release-engineering/pubtools-pulp/blob/master/tests/test_garbage_collect.py, because I see you used private API there, which is bad.

I consider it in scope of pubtools-pulp to encapsulate all logic like "notes.pub_temp_repo exists and is used to mark repo as a temporary", instead of having other projects needing to know that.

@rohanpm rohanpm marked this pull request as ready for review August 13, 2019 17:50
@rohanpm rohanpm requested a review from rajulkumar as a code owner August 13, 2019 17:50
This attribute tells us whether a repository should be considered
"temporary" (i.e. it's created to support execution of some
workflow and should be deleted once that's complete).
@rajulkumar
Copy link
Contributor

Agree that private API shouldn't be used. However, was wondering whether it's fine to add this new attribute to the Repository class based on the "notes" as there are a lot more values in "notes" that might be required in future. Also, this attribute is only required for tests and doesn't affect the actual code.

@rohanpm
Copy link
Contributor Author

rohanpm commented Aug 13, 2019

In my view, it's in scope for this library to encapsulate all the notes we might want to use. The schema in here should be the definitive source when someone is wanting to investigate which notes might be found on our Pulp servers and what do they mean.

About it only being needed for tests - I guess you mean because the fake controller/client can only search on defined fields (via pulp_attrib)?

I think this shows I've made a bit of a design mistake with the search functionality, and I have a plan how to fix it. Right now, to find these repos you have to search for "notes.pub_temp_repo", so you have to know that this note exists and it's not abstracted fully by this new is_temporary attribute. Now I'm thinking you should be able to search by using the field names defined in this library, i.e. search for a repo by field "is_temporary". That way, both the real and test code in pubtools-pulp will only be dealing with "is_temporary" field, and "notes.pub_temp_repo" will genuinely be just an implementation detail of this library.

So, I plan to work on a PR which implements that. We could wait until that's ready and have another look at this if you want to get a better idea what I mean.

@rajulkumar
Copy link
Contributor

I see what you mean. For the idea of encapsulation this change makes sense.

@rohanpm rohanpm merged commit 235eba3 into release-engineering:master Aug 14, 2019
@rohanpm rohanpm deleted the temporary-field branch August 14, 2019 11:21
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.

2 participants