PR#336: Support for epel*-playground branch requests - fedpkg

fedpkg

#336 Support for epel*-playground branch requests

Merged 4 years ago by onosek. Opened 4 years ago by mohanboddu.

mohanboddu/fedpkg epel-playground into master

Support for epel*-playground branch requests

Mohan Boddu • 4 years ago

e33b71e

fedpkg/cli.py

file modified

+28 -6

		`@@ -904,10 +904,23 @@`
		`'a git repository')`

		`pdc_url = config.get('{0}.pdc'.format(name), 'url')`
		`+ # When a 'epel\d' branch is requested, it should automatically request`
		`+ # 'epel\d+-playground' branch.`
		`+ epel_playground = False`
		`if branch:`
		`+ # Check if the requested branch is an epel branch`
		`+ if bool(re.match(r'^(?:epel)\d+$', branch)):`
		`+ epel_playground = True`
		`+`
		`if is_epel(branch):`
		`assert_valid_epel_package(repo_name, branch)`

		`+ # Requesting epel\d-playground branches is not allowed`
		`+ if bool(re.match(r'^(?:epel)\d+-playground$', branch)):`
		`+ raise rpkgError(`
		`+ 'You cannot request {0} branch, as they are '`
		`+ 'created as part of epel branch requests'.format(branch))`
		`+`
		`if ns in ['modules', 'test-modules', 'flatpaks']:`
		`branch_valid = bool(re.match(r'^[a-zA-Z0-9.\-_+]+$', branch))`
		`if not branch_valid:`
		`@@ -942,7 +955,13 @@`
		`branches = [b for b in release_branches`
		`if re.match(r'^(f\d+)$', b)]`
		`else:`
		`- branches = [branch]`
		`+ # If the requested branch is epel branch then also add epel\d+-playground`
		`+ # branch to the request list.`
		`+ # Remove the check for epel version >= 7 when we enable playground for epel7`
		`+ if epel_playground and int(''.join([i for i in branch if re.match(r'\d', i)])) >= 7:`
		`+ branches = [branch, branch+"-playground"]`
		`+ else:`
		`+ branches = [branch]`

		`for b in sorted(list(branches), reverse=True):`
		`ticket_body = {`
		`@@ -965,11 +984,14 @@`

		`# For non-standard rpm branch requests, also request a matching new`
		`# module repo with a matching branch.`
		`- auto_module = (`
		`- ns == 'rpms'`
		`- and not re.match(RELEASE_BRANCH_REGEX, b)`
		`- and not no_auto_module`
		`- )`
		`+ auto_module = None`
		`+ # Dont run auto_module on epel requests`
		`+ if not epel_playground:`
		`+ auto_module = (`
		`+ ns == 'rpms'`
		`+ and not re.match(RELEASE_BRANCH_REGEX, b)`
		`+ and not no_auto_module`
		`+ )`
		`if auto_module:`
		`summary = ('Automatically requested module for '`
		`'rpms/%s:%s.' % (repo_name, b))`

mohanboddu commented 4 years ago

Signed-off-by: Mohan Boddu mboddu@bhujji.com

mohanboddu commented 4 years ago

Please let me know if its good and I will add some comments

rebased onto 20a3e53c743d23ec71e68dd0664ca10984963b60

4 years ago

pretty please pagure-ci rebuild

4 years ago

rebased onto e6bb2f6db617f9210d829bd860d29fd3e043d523

4 years ago

pretty please pagure-ci rebuild

4 years ago

rebased onto e98a9b32228d6aa776ff78b71709d83d79db4892

4 years ago

pretty please pagure-ci rebuild

4 years ago

rebased onto 148d10b1c319a19c7a0957f5e6072afc2e67693a

4 years ago

pretty please pagure-ci rebuild

4 years ago

onosek commented 4 years ago

Thanks, @mohanboddu, I am open to merge it. Do you want to add some additional code/comments?

onosek commented on line 9 of fedpkg/cli.py 4 years ago

So "epel\d-playground" is a correct branch name, right? There was a discussion about it at https://pagure.io/fedpkg/issue/334

mohanboddu commented 4 years ago

@onosek, I am making adjustments to this PR, since we decided to automatically request epel\d-playground when someone requests epel\d.

PR will be updated with notes in few min.

Edited 4 years ago by mohanboddu

rebased onto 27bbff9c755aa78ec748128de0a56dfec22e05c9

4 years ago

pretty please pagure-ci rebuild

4 years ago

rebased onto 9e839b542cd23f1fb1eb54671ee143fa5f0dcd73

4 years ago

smooge commented 4 years ago

onosek yes.. epel\d-playground is the correct name. when we get to epel10 in 2026 it will be epel\d+-playground

rebased onto aee00a4c7e2d724d9bf37b09eda4c1e4d733ca78

4 years ago

pretty please pagure-ci rebuild

4 years ago

sgallagh commented on line 7 of fedpkg/cli.py 4 years ago

Why is this None here but boolean later? Shouldn't it be False?

mohanboddu commented 4 years ago

Why is this None here but boolean later? Shouldn't it be False?

Sorry it got carried from my previous patch, it should be false, you are right.

sgallagh commented 4 years ago

I think we should probably consider disallowing epel\d+-playground from being requested specifically as well. We don't want to allow a situation where only that one is available.

sgallagh commented on line 40 of fedpkg/cli.py 4 years ago

Right now, if I read this correctly, if someone did fedpkg new-branch epel8-playground, epel_playground would be False and it would create a module branch called epel8-playground, which we don't want.

I think we should expressly disallow requesting epel\d+-playground branches and allow them to be created only alongside epel\d+.

rebased onto 59a89a837eeb961c6c9c505d83c86ed6bd38635f

4 years ago

mohanboddu commented 4 years ago

I think we should probably consider disallowing epel\d+-playground from being requested specifically as well. We don't want to allow a situation where only that one is available.

Fixed.

sgallagh commented 4 years ago

LGTM now.

mohanboddu commented 4 years ago

@onosek Please review it now and let me know, thanks.

onosek commented on line 17 of fedpkg/cli.py 4 years ago

A colon is missing. This is why unittests are failing.

rebased onto a08763ba61b79d551d3770aa1017807d3acc3a85

4 years ago

mohanboddu commented 4 years ago

A colon is missing. This is why unittests are failing.

Thanks @onosek , it has been fixed now.

rebased onto 02618f756ee915da322c6ac42918a0c5501c3c78

4 years ago

pretty please pagure-ci rebuild

4 years ago

onosek commented on line 6 of fedpkg/cli.py 4 years ago

minor - epel\d+-playground. A dash is missing.

onosek commented on line 17 of fedpkg/cli.py 4 years ago

'+' should not be at the end of the expression but behind the '\d'.

onosek commented on line 30 of fedpkg/cli.py 4 years ago

Minor. Add '+' behind '\d'. Or use *-playground.

onosek commented 4 years ago

@mohanboddu I am sorry for later review. I had to leave early on Friday.

rebased onto 024fe6bea23d5852342f6c5a275615012185adfd

4 years ago

mohanboddu commented 4 years ago

@mohanboddu I am sorry for later review. I had to leave early on Friday.

I updated it with your comments.

mohanboddu commented on line 33 of fedpkg/cli.py 4 years ago

@onosek I added the check for epel7, as we are planning to roll it initially to epel8 only.

We need to remove it later when we enable it for epel7

onosek commented on line 33 of fedpkg/cli.py 4 years ago

I am not completely sure, what is the purpose of this. But this construction probably needs comparison with a string like '7' (it is currently integer).

onosek commented 4 years ago

BTW thanks for updates for my comments.

rebased onto e33b71e

4 years ago

mohanboddu commented 4 years ago

I am not completely sure, what is the purpose of this. But this construction probably needs comparison with a string like '7' (it is currently integer).

We are only enabling it for epel8 for now and we will definitely have it for epel9 and onwards.

I dont know when we will enable it for epel7, hence the check.

I changed it to int to check the versions, Thanks for the catch, not in a good day today.

onosek commented 4 years ago

Commit eabb5ae fixes this pull-request

Pull-Request has been merged by onosek

4 years ago

onosek commented 4 years ago

I did some additional modifications in the commit eabb5ae. But the result should be the same. I also prepared some unittest for this functionality. Thank you for the contribution, @mohanboddu.

mohanboddu commented 4 years ago

@onosek Yeah, I was trying to avoid change at multiple places when we enable it for epel7, thats all :smile:

onosek commented 4 years ago

I am not completely sure, what is the purpose of this. But this construction probably needs comparison with a string like '7' (it is currently integer).

We are only enabling it for epel8 for now and we will definitely have it for epel9 and onwards.
I dont know when we will enable it for epel7, hence the check.
I changed it to int to check the versions, Thanks for the catch, not in a good day today.

@mohanboddu, I have a additional question. @lsedlar mentioned something which I didn't realize. Currently, the code enables 'epel_playground' for epel branches including version 7 and next. There is an uncertainity, if I have to release as it is and epel_playground will be enabled for epel7. What is a plan for this?

smooge commented 4 years ago

OK thanks for the info. The idea is to have epel7-playground out in the fall after we have gotten epel8 and epel8-playground out the door. The goal would be to follow what we did in epel8 so workflows and tools are the same.