-
-
Notifications
You must be signed in to change notification settings - Fork 404
Support updating label and origin domain of publish #1484
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
base: master
Are you sure you want to change the base?
Conversation
neolynx
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.
excellent, thanks !
wonder if we should add some tests, making sure the values are respected...
49ef3d1 to
2d0d211
Compare
|
@neolynx Thanks for your kindly reminds. Updated the patch and make three test cases for it. ===
|
|
@neolynx Is there anything else supplementary I need to do? Additionally, is there any possibility of adding this patch to Debian:trixie? One of our projects is built on Debian:trixie, and it would be beneficial to our project if this patch could be added to Debian:trixie. |
|
we need a system test to cover the changes in cmd/publish_update.go maybe we can modify an existing one, or add a new one in system/t06_publish/update.py |
I added a test case for it. See the file changes here https://github.com/aptly-dev/aptly/pull/1484/files: Line I checked the newest source code and find the 19th test case is already exist. Compared with https://github.com/aptly-dev/aptly/pull/1522/files, file system/t06_publish/PublishUpdate19Test_gold and system/t06_publish/PublishUpdate19Test_release is not changed but just renamde from xxx19xxx to xxx20xxx. So we just need to rename that very test case( system/t06_publish/update.py), from "PublishUpdate19Test" to "PublishUpdate20Test" is OK. |
|
I renamed on my branch already... but I think there is a problem in the test itself.. investigating... |
|
rebased and renamed the tests... |
Sorry I missed the check function, with the renamed test case, this function is also need to be modified to align with it. 643 def check(self): |
|
ok, I changed that :-) the MRis still in your branch in your repo, you can commit to it as well ... |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1484 +/- ##
==========================================
- Coverage 76.91% 76.61% -0.30%
==========================================
Files 160 160
Lines 14707 14721 +14
==========================================
- Hits 11312 11279 -33
- Misses 2264 2315 +51
+ Partials 1131 1127 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
now it looks great ! :-) could you add yourself to the AUTHORS file and click the check-mark in in MR description ? then we are ready to merge ! |
neolynx
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.
thanks a lot !
Signed-off-by: Zhang Xiao <[email protected]>
Fixes #1007
Requirements
All new code should be covered with tests, documentation should be updated. CI should pass.
Description of the Change
Some times when making the publish, "Label" and "Origin" are not set, or not set correctly thus need to be modified later. With this modification, "Label" and "Origin" can be modified through publish update, no need to drop the re-build the whole publish.
Checklist
AUTHORS