-
Notifications
You must be signed in to change notification settings - Fork 83
Support automatically publishing new repository versions #1337
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
Conversation
5f2549f to
a42ce27
Compare
|
@quba42 As discussed earlier, this currently does only structured publication. I tested the following workflow
Also opened pulp/pulp-cli-deb#223 to add the option to the CLI. Can you take a look? Thanks. |
bf73246 to
2388e50
Compare
|
Before I continue with other aspects of review, I have tested that the basic workflow works as expected: Note how I did not have to create any publications anywhere, because I used |
quba42
left a comment
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.
This already works well. Some remaining ToDos:
Adding release notes
Please add closes #406 to the second line of the commit message to add the issue link.
Then add a file named CHANGES/406.feature to the PR, containing something like:
Added autopublish functionality.
Creates a structured APT publication when used.
Cannot be used to create verbatim publications.
Tests
We need to add some test coverage for this feature, I found the following in pulp_rpm:
https://github.com/pulp/pulp_rpm/blob/main/pulp_rpm/tests/functional/api/test_auto_publish.py
Docs
It would be nice to add some docs for the new workflow, but we can do this under a separate follow up task.
16296f5 to
2ff66e0
Compare
@quba42 Are there any docs available on how to setup the test infrastructure or run tests? I assume simply cloning the repo, activating a venv, installing requirements (from |
I use the https://github.com/pulp/oci_env development environment. The README provides some docs on how to set it up and run tests. I am not sure how complete these docs are for a first time user. If you get stuck, asking in the #pulp-dev channel on matrix is often the fastest way to get help on obscure details. |
2ff66e0 to
ebb8a52
Compare
|
@quba42 I've added tests (well, heavily inspired by the rpm one). Can you take another look? Thanks. |
ebb8a52 to
3a7d0c7
Compare
I am afraid I have three high priority tasks with a deadline right now, so I can't promise anything, but I do have it on my list for as soon as there is an opening. |
3a7d0c7 to
6a6aeac
Compare
@quba42 I know you are probably busy, but to ensure this doesn't fall off your radar, a gentle reminder. 🙂 |
closes pulp#406 Signed-off-by: Balasankar 'Balu' C <[email protected]>
6a6aeac to
e516032
Compare
|
Rebased against latest master |
quba42
left a comment
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.
I have just done a round of review and testing, and everything looks ready to me.
The only reason I cannot yet hit approve and merge, is because I need to ask around one more time what the issue with the "first new migration past the squash migration" is. I have been told that we need to make it depend on 0031_add_domains and not the squash migrations, and all other plugins appear to do this, but I don't understand why.
In my own testing I have not found any problems with upgrading when leaving it as it currently is on this PR. I will find out if and why we need this change and let you know. As soon as I have a resolution I can merge this.
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.
I received feedback about the migration issue that it should work both ways. As such I see nothing holding up this PR. Worst case scenario, we need to change it in a follow up PR later.
@balasankarc Thanks for this work, and for keeping the ball rolling with our change requests! Also thanks for the patience when the ball was in our court.
Note: I will have another look at the accompanying Pulp CLI PR soon. If I find the time, I would also like to follow up by adding a documentation example using the autopublish workflow.
|
@quba42 Thanks. I will open a separate PR for docs. |
@quba42 While that PR is also required, pulp/pulp-cli-deb#223 is the counterpart for this PR. |
Fixes #406