On 04/15/2011 10:29 AM, James Laska wrote:
On Fri, 2011-04-15 at 11:52 -0400, Kamil Paral wrote:
I may be misreading, but I worry about breaking the design principle that the previously 'accepted' set is no longer valid and must be retested. Perhaps this is just confusion over wording though. I'm treating new builds to an existing update as completely new builds. They will have -pending and be in the queue for acceptance like all other -pending builds. I don't understand why clearing all previously -accepted builds is required? They've already passed testing, and at any time may be moved into updates-[testing].
There is a slight difference between adding a build and replacing a build (by a newer version). I was talking about the latter one. But in the end it doesn't matter I guess.
I understand your concern and proposed workflow. But depcheck.py doesn't operate on builds, it operates on updates (as opposed to the depcheck script itself). So let's have a hypothetical update:
Ah yes, I am confused by that. Is the following accurate of the flow?
1. post-koji-build-batch provides a list of builds and schedules depcheck job 2. depcheck.py wrapper calls depcheck test against builds 3. depcheck.py wrapper determines what updates each build maps to 4. depcheck.py wrapper provides feedback for each update based on the net result of builds tested in that update
Slight modification: 1. post-koji-build-batch provides a list of builds and schedules depcheck job 2. depcheck.py downloads all builds in the *-pending tag passed in 3. depcheck.py determines which builds are already passed by querying bodhi for each update 4. depcheck.py wrapper calls depcheck test against the downloaded and classified builds 5. depcheck.py wrapper determines what updates each build maps to 6. depcheck.py wrapper provides feedback for each update based on the net result of builds tested in that update
foo,bar (update)
foo (build) bar (build)
When we detect accepted builds, we detect comment for "foo,bar" and hence we proclaim foo and bar as accepted. When we mark some builds as accepted, we mark the whole update, and that makes all builds inside it accepted.
We don't operate on builds. We simply don't have any approach how to distinguish that foo is accepted, but bar is not (e.g. because it was added later). We do per-update testing. Now. We could probably do per-build testing once we have ResultsDB. Then your approach would be possible.
Still, two problems remain:
- Your approach works when 'bar' is added to the update. But if I replace 'bar-1' by 'bar-2', I can't claim 'foo' is accepted. It can have broken dependency now.
=> If one update may become broken, other updates may become broken too.
If the foo-1.1 and bar-1.1 builds pass depcheck ... we'll want those builds to move out of -pending immediately (or on next push from rel-eng).
If the update is later changed with foo-1.2 (BAD) and bar-1.1, foo-1.2 will be noted as breaking deps, and the update will be commented with a FAILED. However, we have to assume that foo-1.1 and bar-1.1 have already been moved out of -pending and could already be in 'updates-testing'. This scenario seems fine though, since it will keep foo-1.2 in -pending ... preventing the accepted+updates-testing+updates +stable set from breaking repoclosure. To resolve this, the maintainer needs to address the repoclosure problem, which may involve ...
1. Build foo-1.3 to resolve broken dep introduced 2. Build bar-1.2 to satisfy the dep 3. Contact maintainer of another package that needs to be updated 4. Contact rel-eng requesting they remove foo-1.1 and bar-1.1 from updates-testing
Does this mean that a partial update would be pushed (ie foo-1 and bar-1 will be pushed foo-1.2 would be left behind?
Open question - How does bodhi handle -pending tags for builds already in an updated when the update 1) gets a new package added, 2) gets updated packages?
- Even if we could claim that 'foo' is accepted and 'bar' is not, how the RelEng team can push the update? Does it make even sense?
What exactly is wrong about emptying the accepted set, if we don't spam maintainers? Of course I don't want to empty the set if it is not needed, but if we change an already accepted update, I don't see another way.
It's a change in the original design of the test, which may be needed, but makes me nervous as to whether we are breaking any assumptions held throughout the code. I remember having long talks w/ wwoods about never pulling previously accepted builds back into t mix. In some scenarios we discussed, it may not have been the exactly correct way to handle it, but we always maintained repoclosure across accepted + updates-testing + updates + stable.
I think I'm holding on to that original concept because rel-eng pushing is asynchronous. We have no idea when they will take the accepted set and move them to updates-testing. The idea of subtracting (not just adding) to the accepted set seems to expose this to pushing broken updates depending when rel-eng does their pushing.
I'm not disagreeing with you here, but I thought that rel-eng just pushed updates, not builds.
If a package in an update breaks, that update shouldn't be pushed and could affect other updates that are pending.
If we did revoke 'accepted-ness' from an update's build, the burden should still be on the maintainer of that update to fix it but I think that a more interesting question is: "What happens when an accepted update depends on non-broken builds in a failed update?". Can that happen? Wouldn't that still break repoclosure if the passed update was pushed without the non-broken builds it depends on from the failed update?
I'm still wondering if dependencies should be checked at push time (with different comment structure, if any comments are left) instead of just at various times before push.
Also, I think in the future we won't interact with maintainers much. I think depcheck and upgradepath will be mainly RelEng-related tests ("give the proposed updates subset I can push"). The package maintainers will just receive some notice like "Your update was not pushed because...". But those details are not much important right now.
Thanks, James
Tim