Add comment when disabling autopush on negative karma #1292

Open
wants to merge 1 commit into
from

Projects

None yet

2 participants

@trishnaguha
Contributor

Fixes #1291

bodhi/server/scripts/approve_testing.py
+ print('%s has received negative karma' % update.title)
+ text = unicode(config.get('disable_automatic_push_to_stable'))
+ update.comment(db, text, author=u'bodhi')
+ continue
@bowlofeggs
bowlofeggs Feb 17, 2017 Member

I think it would be desirable to make this comment at the moment that the negative karma is disabled, rather than later. This script only runs every 6 hours. I think this would be a good place to add such a comment:

https://github.com/fedora-infra/bodhi/blob/2.4.0/bodhi/server/models/models.py#L1724-L1726

@trishnaguha
trishnaguha Feb 18, 2017 Contributor

Makes sense! I never knew it runs every 6 hours.

@trishnaguha
Contributor

@bowlofeggs Updated :-).

@bowlofeggs

The only thing I think we need to change is to make the new setting optional, so that bodhi can keep working without it.

bodhi/server/models.py
@@ -1832,6 +1832,8 @@ def check_karma_thresholds(self, db, agent):
if self.autokarma and self._composite_karma[1] != 0:
log.info("Disabling Auto Push since the update has received negative karma")
self.autokarma = False
+ text = unicode(config.get('disable_automatic_push_to_stable'))
@bowlofeggs
bowlofeggs Feb 20, 2017 Member

It would be a good idea to put a default value here, so that Bodhi won't crash if this new config value isn't added to the config. I like to follow Semantic Versioning where possible, so this would be an easy way to avoid a backwards incompatible change.

I think this might be as simple as changing the above line to something like this:

default_comment = u'Bodhi is disabling automatic push to stable due to negative karma. The maintainer may manually if they determine that the issue is not severe.'
text = unicode(config.get('disable_automatic_push_to_stable', default_comment))
production.ini
@@ -19,6 +19,8 @@ stablekarma_comment = This update has reached the stable karma threshold and wil
testing_approval_msg_based_on_karma = This update has reached the stable karma threshold and can be pushed to stable now if the maintainer wishes.
not_yet_tested_msg_based_on_karma = This update has not reached the stable karma threshold.
+disable_automatic_push_to_stable = Negative karma received, disabling automatic push to stable. Please push manually if you decide the issue is not severe.
@bowlofeggs
bowlofeggs Feb 20, 2017 Member

I suggest a slightly different wording. Perhaps something like:

Bodhi is disabling automatic push to stable due to negative karma. The maintainer may manually if they determine that the issue is not severe.

This is at your option.

@trishnaguha
trishnaguha Feb 21, 2017 Contributor

Either of them works for me.

development.ini.example
@@ -26,6 +26,8 @@ stablekarma_comment = This update has reached the stable karma threshold and wil
testing_approval_msg_based_on_karma = This update has reached the stable karma threshold and can be pushed to stable now if the maintainer wishes.
not_yet_tested_msg_based_on_karma = This update has not reached stable karma threshold.
+disable_automatic_push_to_stable = Negative karma received, disabling automatic push to stable. Please push manually if you decide the issue is not severe.
@bowlofeggs
bowlofeggs Feb 20, 2017 Member

I suggest a different wording here, also at your option.

@@ -3088,6 +3088,9 @@ def test_disable_autopush_non_critical_update_with_negative_karma(self, publish,
up.comment(self.db, u'Failed to work', author=u'ralph', karma=-1)
up = self.db.query(Update).filter_by(title=resp.json['title']).one()
+ expected_comment = unicode(config.get('disable_automatic_push_to_stable'))
+ self.assertEquals(up.comments[2].text, expected_comment)
@bowlofeggs
bowlofeggs Feb 22, 2017 Member

One last thing to fix and I think we'll be good to go:

This test doesn't pass if development.ini doesn't have the new disable_automatic_push_to_stable setting. An easy way to fix it would be to put the default comment here in the call to config.get() too. It would be nice to allow development checkouts with existing development.ini files to keep working ☺

In the long run, I'd like to adjust bodhi.server.config so that all settings have sane defaults in a dictionary that is then overwritten by whatever is found in the config files. This way, all settings always have a default and we can always use config.get with one argument and can guarantee that it will be defined, regardless of what is in the ini files.

@trishnaguha
trishnaguha Feb 23, 2017 Contributor

Pushed :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment