-
Notifications
You must be signed in to change notification settings - Fork 78
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
Allow source files to publish at by-hash paths #1061
Conversation
11c48ab
to
7b74959
Compare
I've confirmed that source files have their metadata published to /source/by-hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified that this works. I'm inclined to merge this. In the end though it would be great to have tests for apt-by-hash
in general not necessary source but all packages. But I can create an extra issue for this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acheng-01 I am going to second what @hstct has said.
Since I do not naturally use/manually test the APT by hash story, I will not notice if anything breaks unless it has test coverage, so it is really in the interest of those using the story to add that test coverage.
That being said, I am willing to merge this as is (especially since we did not have the common curtsy to give an initial review for this in a timely manner).
@acheng-01, @daviddavis (if you are involved with this): Please let us know how you want to proceed.
Yea, I thought we had tests for apt-by-hash but it looks like we just added one for caching content to be served for apt-by-hash. I think adding a test for apt-by-hash makes sense. I don't have a preference if we add it in this PR or a followup PR. Also, I might suggest we add a general apt-by-hash test and not one for the source files since source files are a pain to test with. Also, how do we enable APT_BY_HASH in the test environment. Just set it here? |
@daviddavis Yea I think that should do it. It might be sufficient to add a test here and see if we can just download the correct files from the |
Thanks for chiming in everyone. I'll look into adding tests in within the next week or so as part of this PR! |
60f5a6c
to
d4d1e43
Compare
d4d1e43
to
1234129
Compare
Apologies that getting this test in has taken so long, but it's finally here! Please let me know if there is anything I should change with this test - otherwise it is passing on my end. Thanks everyone for your patience :) |
@quba42 @hstct The new test here is passing locally for me and @acheng-01 but it seems to fail in the CI. Any ideas? |
My guess is that the CI scripts need to be regenerated given the change to template_config.yml but I have no idea how to do that anymore. |
What you need to do is have both pulp_deb and plugin template cloned locally. Make sure you check out this PR branch in pulp_deb. In the pugin_template repo make sure you check out the commit referenced here (e.g.: |
1234129
to
384169d
Compare
@quba42 I did the following:
There are a lot of files changed after those steps and I'm not sure if they're correct. For some reason the gitref says unknown now. Is that supposed to happen? |
Never mind, @daviddavis helped me identify what I did wrong in the process. I did the template thing again and things look right now. Could use a second set of eyes to verify this though. |
8399eb9
to
f756092
Compare
e8a6384
to
f3f4ad2
Compare
I'm not entirely sure why this test is not working in the CI pipeline. It appears that the publication isn't created successfully somehow? The I haven't been able to repro this failure locally so it's been difficult trying to understand why this is happening. The way that this test initiates the repo, publication, and distribution should be relatively similar to the other two content tests, with the only difference being that I'm passing in "sync_sources": True for remote args. Maybe that's what's causing this test to behave differently? Would greatly appreciate if anyone might know why this is happening :) |
One more find in the logs:
I was expecting |
@acheng-01 I was able to reproduce the error locally. If you rebase your branch against main, your test fails. I believe something about this change that was merged yesterday is causing your change to fail. |
Fix a source publishing bug where the code was looping through all release components instead of just the release components for the distribution when attempting to publish source files for a specific distribution component. This raised a key error that was found in @acheng-01's PR which added a test for source publishing: pulp#1061
Fix a source publishing bug where the code was looping through all release components instead of just the release components for the distribution when attempting to publish source files for a specific distribution component. This raised a key error that was found in @acheng-01's PR which added a test for source publishing: pulp#1061 [noissue]
f3f4ad2
to
cfdab9a
Compare
Bug fix for awareness. I implemented @daviddavis's change so that the ci tests are passing for this PR, but let me know if I should revert it. Again, huge thanks for the assist! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea, sorry I missed that when adding the change last week. I merged @daviddavis's PR.
Thanks for implementing the test. Reviewed your changes again, they are looking good! You can revert that patch from yesterday and everything should be good to go!
cfdab9a
to
dfa756a
Compare
dfa756a
to
02af514
Compare
@hstct I did another rebase on main and it looks like all the tests are passing. Ready for merge! |
Awesome! Thanks for this contribution! |
Refactored the previous AptByHash code to extend publishing at by-hash paths for source files.
Closes #1059