Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state #901

trishnaguha · 2016-09-02T07:06:32Z

Fixes #880

bowlofeggs · 2016-09-16T17:23:30Z

bodhi/models/models.py

@@ -1077,6 +1077,16 @@ def set_request(self, db, action, username):
                update=self, agent=username))
            return

+        # If an update with pending status receives negative karma, make sure it gets obsoleted
+        if self.status is UpdateStatus.pending and self.request is UpdateRequest.testing \
+            and self.karma < 0:


I'm thinking that we should only do this if autokarma is turned on. Do you agree? If so, rather than testing for negative karma perhaps we could test to see if the negative karma threshold is reached.

@bowlofeggs Yeah I agree with autokarma turned on.
Okay one more query regarding negative karma. Do we want to check negative karma from recent comment like we did has_negative_karma or just checking unstable_karma(negative_karma) threshold would be choice here?

@trishnaguha I think just checking that method should be fine. Does that method not account for users who changed their minds? If it doesn't, let's file a separate issue (and separate PR) for that and we can fix it later.

bowlofeggs · 2016-09-16T17:28:30Z

Can you rebase your commit?

bowlofeggs · 2016-09-16T17:36:26Z

bodhi/tests/functional/test_updates.py

@@ -1955,6 +1955,33 @@ def test_revoke_action_for_testing_request(self, publish, *args):
        publish.assert_called_with(
                topic='update.request.revoke', msg=mock.ANY)

+    @mock.patch(**mock_valid_requirements)
+    @mock.patch('bodhi.notifications.publish')
+    def test_obsolete_update_on_pending_state(self, publish, *args):


I'm not familiar with the rest of the tests in this area, so can you please ensure that these five conditions are covered by tests that are either new or existing? There are 4 conditions that will be tested in the new if statement, so we could conceivably test 5 conditions. The first is a positive case (basically, what you have here but modified to set the autokarma and reach the negative threshold), and the other four just ensure the negative cases:

The update is pending, is marked for testing, has autokarma true, and has reached the negative karma threshold. This update should get obsoleted.

All of the first test, but the update isn't pending.

All of the first test, but the update isn't marked for testing (this one should probably also be obsoleted, but by another function in the code. I also bet there's already a test for this.)

All of the above, but autokarma is off.

All of the above, but the karma is not beyond the thresholds.

You don't necessarily have to write the above tests if they already exist, but please ensure that they do exist.

trishnaguha · 2016-09-20T13:41:11Z

Updated the patch. I will make sure that we have enough testcases for the patch.

trishnaguha · 2016-09-21T09:09:45Z

@bowlofeggs I have updated the test set. Can you please explain about the test you mentioned at no. 3?

bowlofeggs · 2016-09-25T18:14:28Z

On Wed, 2016-09-21 at 02:09 -0700, Trishna Guha wrote:

@bowlofeggs I have updated the test set. Can you please explain about
the test you mentioned at no. 3?

Basically that we should test behavior when self.request is not UpdateRequest.testing, and make sure the right thing happens.

Overall, I want to ensure we hit all the bits that are anded in the if
statement.

trishnaguha · 2016-09-25T20:54:36Z

@bowlofeggs Sure I got you :).

trishnaguha · 2016-09-26T11:43:42Z

@bowlofeggs I just debugged that When we are not making request to testing on pending status it by default gets set to testing. Does it make sense to make request to None for this testcase? if that is the case, karma is not going to affect the update unless the request is testing. the status still remains as pending because of request being None.

bowlofeggs

I think just two more changes and we'll be good to go:

Indent that one line so it is visually separated from the code below it.
Clarify the docblocks on the two very similar tests.

Nice work!

bowlofeggs · 2016-09-27T16:18:53Z

bodhi/server/models/models.py

+        threshold, make sure it gets obsoleted.
+        """
+        if self.autokarma and self.status is UpdateStatus.pending and self.request is UpdateRequest.testing \
+            and self.unstable_karma not in (0, None) and self.karma <= self.unstable_karma:


Can you add one more level of indentation to this line so that it's visually separated from the code below?

bowlofeggs · 2016-09-27T16:51:03Z

bodhi/tests/server/functional/test_updates.py

+        up = self.db.query(Update).filter_by(title=nvr).one()
+        self.assertEquals(up.karma, -1)
+        self.assertEquals(up.status, UpdateStatus.pending)
+        self.assertNotEquals(up.status, UpdateStatus.obsolete)


I think the first assertion implies the second assertion, so if it were me I would leave only the assertEqual and drop the assertNotEquals. That said, there's also obviously no direct harm in leaving this assertion, but less lines of code is preferable in my book ☺ You can leave it as is if you want, just a suggestion.

bowlofeggs · 2016-09-27T16:53:49Z

bodhi/tests/server/functional/test_updates.py

+
+        up = self.db.query(Update).filter_by(title=nvr).one()
+        self.assertEquals(up.karma, -1)
+        self.assertNotEquals(up.status, UpdateStatus.obsolete)


Same idea here, I think it's slightly cleaner to have one less line of code and keep only the assertEquals instead of this assertNotEquals. Again, it's preference so you can leave it if you want.

bowlofeggs · 2016-09-27T16:56:17Z

bodhi/tests/server/functional/test_updates.py

+    def test_obsolete_if_unstable_with_autopush_enabled_when_testing(self, publish, *args):
+        """
+        Send update to obsolete state if it reaches unstable karma threshold on
+        testing state when Autopush is enabled.


It seems that the main difference between this test and the first one is that this one has up.request set to stable and the other has up.request set to testing. Can you clarify that difference in the docblocks of both tests?

The first test makes sure that the update doesn't go to testing(should be obsoleted) if it receives negative karma on the pending state where request is testing when autopush is enabled.

This test makes sure that the update doesn't got to stable(should be obsoleted) if it receives negative karma on the testing state where request is stable when autopush is enabled.

Yes I will update the PR with that :).

…reshold on pending state

trishnaguha · 2016-09-27T17:37:41Z

Thanks :-).

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by fedora-infra#901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, fedora-infra#1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before fedora-infra#901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. fedora-infra#901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after fedora-infra#901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer fedora-infra#1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by #901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, #1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com>

@bowlofeggs

Before #901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. #901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after #901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer #1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com>

`obsolete_if_unstable` was introduced by #901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, #1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com> (cherry picked from commit 3f21fd3)

@bowlofeggs

Before #901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. #901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after #901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer #1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com> (cherry picked from commit 25200ad)

`obsolete_if_unstable` was introduced by #901 to cover one path: obsoleting a 'pending' update if it reached its negative karma threshold. At the time, `check_karma_thresholds` only reached its negative karma check for updates in 'testing' state, it did not run on updates in 'pending' state. However, #1556 extended `check_karma_thresholds` to also run on updates in 'pending' state, which actually rendered `obsolete_if_unstable` entirely redundant, except in one case - calling `comment()` with `check_karma` set to `False` - which was introduced solely to make it possible to test `obsolete_if_unstable`. Signed-off-by: Adam Williamson <awilliam@redhat.com> (cherry picked from commit 3f21fd3)

@bowlofeggs

Before #901, we obsoleted updates that reached their unstable_karma threshold regardless of autokarma setting, but only for updates in 'testing' status, not 'pending' status. #901 intended to fix that. In the review, @bowlofeggs said "we should only do this [obsolete updates in pending state that reach their unstable_karma threshold] if autokarma is turned on", but he did not apparently realize that was inconsistent with the existing behavior in the testing state. So after #901, we would obsolete updates from pending only if autokarma was enabled, but we would always obsolete updates from testing regardless of the autokarma setting. This behaviour remained the same afer #1556. I don't think it makes any sense to preserve this inconsistent behavior. This PR comes down on the side of always obsoleting regardless of the setting of autokarma, as I think in practice this happens most often to updates in 'testing' state not updates in 'pending' state, so it's the most similar behaviour. It's also the older behaviour. It would be easy to flip it to only ever unpush if autokarma is enabled, if we decide that's better. Signed-off-by: Adam Williamson <awilliam@redhat.com> (cherry picked from commit 25200ad)

bowlofeggs reviewed Sep 16, 2016

View reviewed changes

trishnaguha force-pushed the obsolete-package branch 2 times, most recently from e6bb0bb to 197198c Compare September 20, 2016 13:39

trishnaguha changed the title ~~Make sure that an update is obsoleted if it has negative karma on pending state~~ Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state Sep 21, 2016

trishnaguha force-pushed the obsolete-package branch from 197198c to c8b419c Compare September 21, 2016 09:07

bowlofeggs suggested changes Sep 27, 2016

View reviewed changes

Make sure that an update is obsoleted if it reaches unstable karma th…

f033c74

…reshold on pending state

trishnaguha force-pushed the obsolete-package branch from c8b419c to f033c74 Compare September 27, 2016 17:27

bowlofeggs approved these changes Sep 27, 2016

View reviewed changes

bowlofeggs merged commit 9c55164 into fedora-infra:develop Sep 27, 2016

trishnaguha deleted the obsolete-package branch September 27, 2016 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state #901

Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state #901

trishnaguha commented Sep 2, 2016

bowlofeggs Sep 16, 2016 •

edited

Loading

trishnaguha Sep 19, 2016 •

edited

Loading

bowlofeggs Sep 19, 2016

bowlofeggs commented Sep 16, 2016

bowlofeggs Sep 16, 2016

trishnaguha commented Sep 20, 2016

trishnaguha commented Sep 21, 2016

bowlofeggs commented Sep 25, 2016

trishnaguha commented Sep 25, 2016

trishnaguha commented Sep 26, 2016 •

edited

Loading

bowlofeggs left a comment

bowlofeggs Sep 27, 2016

bowlofeggs Sep 27, 2016

bowlofeggs Sep 27, 2016

bowlofeggs Sep 27, 2016

trishnaguha Sep 27, 2016 •

edited

Loading

trishnaguha commented Sep 27, 2016

Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state #901

Make sure that an update is obsoleted if it reaches unstable karma threshold in pending state #901

Conversation

trishnaguha commented Sep 2, 2016

bowlofeggs Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

trishnaguha Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

bowlofeggs Sep 19, 2016

Choose a reason for hiding this comment

bowlofeggs commented Sep 16, 2016

bowlofeggs Sep 16, 2016

Choose a reason for hiding this comment

trishnaguha commented Sep 20, 2016

trishnaguha commented Sep 21, 2016

bowlofeggs commented Sep 25, 2016

trishnaguha commented Sep 25, 2016

trishnaguha commented Sep 26, 2016 • edited Loading

bowlofeggs left a comment

Choose a reason for hiding this comment

bowlofeggs Sep 27, 2016

Choose a reason for hiding this comment

bowlofeggs Sep 27, 2016

Choose a reason for hiding this comment

bowlofeggs Sep 27, 2016

Choose a reason for hiding this comment

bowlofeggs Sep 27, 2016

Choose a reason for hiding this comment

trishnaguha Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

trishnaguha commented Sep 27, 2016

bowlofeggs Sep 16, 2016 •

edited

Loading

trishnaguha Sep 19, 2016 •

edited

Loading

trishnaguha commented Sep 26, 2016 •

edited

Loading

trishnaguha Sep 27, 2016 •

edited

Loading