Added: Support for per-component/arch Release files #1275
Conversation
hstct
left a comment
hstct
left a comment
There was a problem hiding this comment.
This looks promising so far!
If it's possible it would be great if you could add or extend a publication test that verifies the creation of the file and it's contents.
|
Thanks for the review!
I will try to add a test for this. I just don't have much experience with pulp yet 🙂 |
|
@hstct i am struggling with implementing the test case for my change. As there are more changes in the pipeline, would it be possible to get in contact directly (e.g. via call or IM) to get a primer on how the tests work? |
|
There is a pulp-debian channel on matrix where you can reach us too. I'm okay with chatting there or answer any questions you have here. |
|
Update: The current way of creating those release files during publish is not clean and complicated to test. The current plan is to rework the PR to properly sync and publish the Legacy per-component-and-architecture Release files like other files. |
hstct
left a comment
hstct
left a comment
There was a problem hiding this comment.
The main worry here is the manual building of the strings in the release file. Using the deb822 helper should fix it.
Afterwards I'm inclined to use this as a minimal implementation, meaning we don't need an extra test for it.
We can worry on a proper implementation another time, this would entail creating a own model for these Release file.
| # Publish per-component/architecture Release files | ||
| for release_path in self.release_file_paths.values(): | ||
| release = PublishedMetadata.create_from_file( | ||
| publication=self.parent.publication, file=File(open(release_path, "rb")) |
There was a problem hiding this comment.
I'm not sure we need to loop over this. Can't we just say release.save() ? maybe not call it release.
this way we don't need the self.release_file_paths variable
There was a problem hiding this comment.
Great idea and i really appreciate the help. I updated the PR with the file creation in its own function as you suggested in the other change.
I still used the self.release_file_paths variable for publishing because i don't fully understand what you mean with release.save(). I still would want to change it.
Yea, I had some more time looking into this and I think a minimal solution should be the goal of this PR. It's close already! |
|
Does this impact all publications or just verbatim ones? I am wondering if a setting to enable/disable publishing of these files makes sense since I don't think we need them and they'll add more traffic for us, etc. |
This change affects all publications. However, from a sync perspective nothing changes as pulp has always fetched these files from the remote repository. But yea okay it might have consequences for the published repository now serving more files. Normally we have flags on the remote but I don't know if there are consequences if we don't sync these files anymore. I don't mind if we add a flag for this on a Publication. Would that be okay? |
|
We don't sync at all so I am guessing we don't have these files in our system. A flag on a publication would mean having it set every time one publishes? I think a global setting might be easier at least for us in this regard. But I suppose some users might want to customize it per publication. A flag on a publication would work for us. |
|
We can set the default to false so not setting the flag won't change current behavior |
|
@hstct will you be able to take a look here soon? |
|
@DEiselt yea I put it on my list for this week. I kinda lost track of this one, sorry about that. |
|
@DEiselt sorry for taking longer than expected. I think most of the this change is fine as is. But I noticed you are trying to merge it into the 3.5 branch. Could you change this to the main branch? It might require some rebasing. I can add a backport to 3.5 for this change after we merge it on the main branch. |
|
@hstct do you typically backport features? I thought Pulp projects typically created new z-stream/minor releases for new features. |
Oh, yea sorry you are right, I wasn't thinking and treated it as a bugfix. We don't usually backport features without good reason |
So how would we continue here? Should i still rebase to the main branch and we just don't add it to 3.5? |
|
It needs to go to the main branch either way, we cannot add a feature into a release branch and not have it in the latest version. With the next release this feature will be available. If there is a good reason to backport the feature we will take this under consideration but it's not something we normally do. |
|
That's fine. We use this as a patch already. I initially opened this PR against 3.5 because that was the version i developed it for and created the patch. But we can use the patch until our installation gets updated to a version which includes this change. I will update the PR to use the main branch and squash the commits. Sorry if there was some confusion and thank you for your patience. I haven't done many upstream contributions yet. |
DEiselt
force-pushed
the
3.5_per_release_arch
branch
from
b1da152 to
32654f6
Compare
DEiselt
force-pushed
the
3.5_per_release_arch
branch
from
a87b8e9 to
07228d9
Compare
Rework creation of legacy release files. Restore CHANGES.md Add flag to toggle publish of legacy release files. Reformat files
DEiselt
force-pushed
the
3.5_per_release_arch
branch
from
548cb4e to
8d2669b
Compare
|
@hstct i now updated the PR to compare to main and rebased. So from my side, the PR is ready for a "final" review. Note: i'm not sure why the "Update Changelog" commit is here but looking at the changed files, only my changes are part of the PR. |
|
@hstct do you have an explanation why those tests are failing? I tried to reproduce it locally but don't really understand the error. |
3 participants
closes: #1175