Skip to content
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

Add autopush boolean to an update. #3090

Merged
merged 1 commit into from Jun 20, 2019

Conversation

4 participants
@cverna
Copy link
Member

commented Mar 25, 2019

This commit adds an autopush boolean to an update.
When this boolean is true the approve_testing cronjob
will push the update to stable if it has meet the
testing requirements (time).

Signed-off-by: Clement Verna cverna@tutanota.com

@cverna cverna requested a review from fedora-infra/bodhi as a code owner Mar 25, 2019

@cverna cverna force-pushed the cverna:auto_time_push branch from 1d17c2d to cd0b45d Mar 25, 2019

@bowlofeggs
Copy link
Member

left a comment

I think we should also include the date to push in this. If we don't, we have to do it in two stages, the second of which will require a major release of Bodhi, and the extra overhead of tracking that work outweighs the relatively little work it takes to add it.

@cverna cverna force-pushed the cverna:auto_time_push branch 4 times, most recently from 1fa949a to b460940 Mar 26, 2019

@cverna

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Ok , I have update the PR with the stable_days field in the database so that we can store the user input on the number of days in testing required.

If this looks good, I ll work on adding support to the cli.

@cverna cverna force-pushed the cverna:auto_time_push branch from b460940 to 3fd90bc Apr 1, 2019

@cverna cverna changed the title WIP - Add autopush boolean to an update. Add autopush boolean to an update. Apr 1, 2019

@cverna cverna force-pushed the cverna:auto_time_push branch from 3fd90bc to c07cb5d Apr 1, 2019

Show resolved Hide resolved bodhi/client/__init__.py Outdated
Show resolved Hide resolved .../versions/b1fd856efcf6_add_autopush_boolean_and_stable_days_to_update.py Outdated
Show resolved Hide resolved .../versions/b1fd856efcf6_add_autopush_boolean_and_stable_days_to_update.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/templates/new_update.html Outdated

@cverna cverna force-pushed the cverna:auto_time_push branch 4 times, most recently from b8b7084 to 9e0f1a3 Apr 11, 2019

Show resolved Hide resolved bodhi/server/models.py Outdated
@@ -3339,7 +3365,7 @@ def met_testing_requirements(self):
if min_num_days:
if not self.meets_testing_requirements:
return False
else:
elif not self.autopush:

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Apr 15, 2019

Member

Why this change?

This comment has been minimized.

Copy link
@cverna

cverna Apr 15, 2019

Author Member

In case of rawhide min_num_days will be 0 which would make us think that we already met the testing requirements.

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Apr 15, 2019

Member

If the requirement is 0, then we have met the requirements in that case. I think we should keep the concept of autopush and requirement separate. The push date isn't a requirement, it's just an automation.

This comment has been minimized.

Copy link
@cverna

cverna Apr 15, 2019

Author Member

Ok that makes sense to me, but for some reason in the approve_testing script, we do this (see below) if we have met the requirements, which mean we will never push the rawhide updates.

https://github.com/fedora-infra/bodhi/blob/develop/bodhi/server/scripts/approve_testing.py#L85

The script might be wrong or not, I am not sure 😝

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Apr 15, 2019

Member

@cverna ah yes, I am familiar with that and do not like it!

I think the point of it is to avoid writing a comment on updates that could be pushed every time the script runs, so it's trying to detect if the update has already been commented on. It's not a good way to do that, but I think that's what it's trying to do, if that makes sense.

I think it'd be better if there were:

  1. A formal way to mark a Bodhi comment as being the comment that says this. Right now, it checks if the comment was authored by bodhi and has that particular text (ewww). It'd be better if the Comment model had some metadata/enum to mark certain Bodhi comments as being of that type - then we can more cleanly detect whether Bodhi has already added that comment to an update.
  2. I don't love that we have met_testing_requirements() and meets_testing_requirements(). The names different in tense, of course, but really I think we just want to make sure we don't comment repeatedly.

Anyways, we don't need to solve these nasty problems in this pull request - I'm just trying to give you some context for what this code seems like it's trying to do. It's confusing…

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Apr 15, 2019

Member

Another hilarious thing about those two functions is that one of them calls the other!

Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/tests/server/services/test_updates.py Outdated
@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

@cverna cverna force-pushed the cverna:auto_time_push branch 6 times, most recently from fa9d502 to d0539c9 Apr 15, 2019

@@ -3339,7 +3365,7 @@ def met_testing_requirements(self):
if min_num_days:
if not self.meets_testing_requirements:
return False
else:
elif not self.autotime:

This comment has been minimized.

Copy link
@bowlofeggs

bowlofeggs Apr 17, 2019

Member

I still think we should not use the new boolean to determine whether an update has met testing requirements - we need to keep the concept of requirements and automation machinery separate.

This comment has been minimized.

Copy link
@cverna

cverna Apr 18, 2019

Author Member

Yes I think if we get #3158 merged before this one then we don't need this change.
Otherwise I can manage this special case in the script instead of here.

@cverna cverna force-pushed the cverna:auto_time_push branch 2 times, most recently from 21b8bc9 to 155988e Apr 24, 2019

@cverna cverna force-pushed the cverna:auto_time_push branch from 155988e to 24378c4 May 2, 2019

@bowlofeggs bowlofeggs force-pushed the cverna:auto_time_push branch from fae7f39 to 9b51c78 Jun 10, 2019

Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/models.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py Outdated
Show resolved Hide resolved bodhi/server/scripts/approve_testing.py

@cverna cverna force-pushed the cverna:auto_time_push branch 3 times, most recently from 3864630 to 9f659be Jun 12, 2019

@abompard

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Please squash the two commits and you should be good to go.

@cverna cverna force-pushed the cverna:auto_time_push branch 2 times, most recently from 1438ec7 to 0c2b04c Jun 18, 2019

comment.text.startswith('This update has reached') and \
'and can be pushed to stable now if the ' \
'maintainer wishes' in comment.text:
comment.text == 'This update can be pushed to stable now if the maintainer wishes':

This comment has been minimized.

Copy link
@sebwoj

sebwoj Jun 18, 2019

Collaborator
  1. We can use value from testing_approval_msg config setting instead of hardcoding str. This way we can fix #1014.
  2. It looks a little bit backward incompatible, because current comment text is different. On the other hand, it turns out that this change will only cause Bodhi to comment about update readiness twice.

This comment has been minimized.

Copy link
@cverna

cverna Jun 19, 2019

Author Member
1. We can use value from `testing_approval_msg` config setting instead of hardcoding str. This way we can fix #1014.

Yes good idea, I ll change that.

2. It looks a little bit backward incompatible, because current comment text is different. On the other hand, it turns out that this change will only cause Bodhi to comment about update readiness twice.

Yes there is a risk that for a some updates Bodhi will comment twice, to me this is acceptable. What do @bowlofeggs and @abompard think about that ?

This comment has been minimized.

Copy link
@abompard

abompard Jun 19, 2019

Member

What about changing the first test to .startswith('This update ') and the second to 'can be pushed to stable now if the maintainer wishes'? This way it'll find the new version of the comment too.

This comment has been minimized.

Copy link
@cverna

cverna Jun 19, 2019

Author Member

@abompard that works for me :-)

@cverna cverna force-pushed the cverna:auto_time_push branch 4 times, most recently from 9bc9e5c to c744d7f Jun 19, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jun 19, 2019

@abompard abompard self-assigned this Jun 19, 2019

@abompard
Copy link
Member

left a comment

Yep, looks good to me

@abompard abompard requested review from bowlofeggs and sebwoj Jun 20, 2019

All requested changes were addressed.

@cverna cverna force-pushed the cverna:auto_time_push branch from cd3237d to bf38ddc Jun 20, 2019

text = str(
config.get('testing_approval_msg') % update.mandatory_days_in_testing)
update.comment(db, text, author='bodhi')
update.comment(db, str(config.get('testing_approval_msg')), author='bodhi')

This comment has been minimized.

Copy link
@sebwoj

sebwoj Jun 20, 2019

Collaborator

Just for curiosity: Do we really need this type cast here? testing_approval_msg setting already has: 'validator': str

@sebwoj
Copy link
Collaborator

left a comment

Just one question.

@@ -488,6 +483,280 @@ def test_usage(self, stdout, exit):
'usage: nosetests <config_uri>\n(example: "nosetests development.ini")\n')
exit.assert_called_once_with(1)

def test_autotime_update_meeting_test_requirements_gets_pushed(self):

This comment has been minimized.

Copy link
@sebwoj

sebwoj Jun 20, 2019

Collaborator

Indentation level here. Unit test are failing.

Add autopush and stable_days to an update.
This commit adds an autopush boolean and stable_days integer
to an update. When this boolean is true the approve_testing
cronjob will push the update to stable if it has meet the
testing requirements (stable_days).

stable_days cannot be smaller than the release
mandatory_days_in_testing.

Signed-off-by: Clement Verna <cverna@tutanota.com>

Use only stable approval comment.

This commit simplify how bodhi communicates that an update
is ready to be pushed to stable. Instead of including the reason
why an update is ready to be pushed (karma or time) bodhi
just informs that the update can now be pushed.

Signed-off-by: Clement Verna <cverna@tutanota.com>

@cverna cverna force-pushed the cverna:auto_time_push branch from bf38ddc to 4dbfdb4 Jun 20, 2019

@mergify mergify bot merged commit 9be769b into fedora-infra:develop Jun 20, 2019

28 of 30 checks passed

continuous-integration/jenkins/pr-head This commit is being built
Details
rawhide-integration running
Details
DCO DCO
Details
Summary 2 rules match and 4 potential rules
Details
f29-build
Details
f29-diff-cover
Details
f29-docs
Details
f29-integration
Details
f29-integration-build
Details
f29-unit
Details
f30-build
Details
f30-diff-cover
Details
f30-docs
Details
f30-integration
Details
f30-integration-build
Details
f30-unit
Details
pip-build
Details
pip-diff-cover
Details
pip-docs
Details
pip-flake8
Details
pip-integration
Details
pip-integration-build
Details
pip-mypy
Details
pip-pydocstyle
Details
pip-unit
Details
rawhide-build
Details
rawhide-diff-cover
Details
rawhide-docs
Details
rawhide-integration-build
Details
rawhide-unit
Details

CI Gating automation moved this from Need Review to Merged to develop Jun 20, 2019

@bowlofeggs

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.