Bug 2239790 - Review Request: mod_proxy_cluster - JBoss mod_proxy_cluster for Apache httpd
Summary: Review Request: mod_proxy_cluster - JBoss mod_proxy_cluster for Apache httpd
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tomas Korbar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-09-20 08:11 UTC by huwang
Modified: 2023-10-11 07:01 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-10-11 07:01:41 UTC
Type: ---
Embargoed:
tkorbar: fedora-review+


Attachments (Terms of Use)
changes (6.84 KB, patch)
2023-09-22 09:09 UTC, Tomas Korbar
no flags Details | Diff

Description huwang 2023-09-20 08:11:39 UTC
Spec URL: https://huwang.fedorapeople.org/mod_proxy_cluster.spec
SRPM URL: https://huwang.fedorapeople.org/mod_proxy_cluster-1.3.19-1.fc38.src.rpm
Description: JBoss mod_proxy_cluster for Apache httpd.
Fedora Account System Username: huwang

Comment 1 huwang 2023-09-20 08:14:06 UTC
The scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106420523

Comment 2 Tomas Korbar 2023-09-22 09:09:17 UTC
Created attachment 1990028 [details]
changes

Comment 3 Tomas Korbar 2023-09-22 09:21:55 UTC
Hi huwang,
I reviewed spec file and did some changes that you can see in the attached patch.
I'll try to clarify them:

1. I changed packaging of your selinux policy to conform to https://fedoraproject.org/wiki/SELinux/IndependentPolicy#Creating_Custom_Product_Policies
2. Unified the indenation to general use of spaces
3. Removed the buildroot specification as it is not neccessary
4. Changed source URL to be more straightforward and used autosetup macro accordingly
5. Removed the apxs macros as i was not sure whether they are neccessary (please return them if they are truly necessary but package builds and installs fine without them)
6. Defined the aplipdir on top of the spec so it can be defined just once
7. Updated build section so it uses macros for configure script and parallel build
8. Removed %defattr(0644,root,root,0755) as it is not necessary
9. Added %license record to %files section
10. Updated License tag so it conforms to SPDX naming
11. Added requirement of http_port_t type to your selinux policy

Now i have a few questions. On github i see that the latest version is 2.0.3.
Is there a reason why the package ships 1.3.19?
Also i see that there are excluded archs i686 i386. Is it necessary?
If it truly is then you will have to file bugs for both of them after the release
and explain why is it so.

Looking forward to getting your opinion.

Comment 4 huwang 2023-09-30 12:14:51 UTC
Hi Tomas,
Thanks for your review.
I agree with your changes and update the spec file and srpm here:
Spec URL: https://huwang.fedorapeople.org/mod_proxy_cluster.spec
SRPM URL: https://huwang.fedorapeople.org/mod_proxy_cluster-1.3.19-1.fc38.src.rpm

(In reply to Tomas Korbar from comment #3)
> Hi huwang,
> I reviewed spec file and did some changes that you can see in the attached
> patch.
> I'll try to clarify them:
> 
> 1. I changed packaging of your selinux policy to conform to
> https://fedoraproject.org/wiki/SELinux/
> IndependentPolicy#Creating_Custom_Product_Policies
> 2. Unified the indenation to general use of spaces
> 3. Removed the buildroot specification as it is not neccessary
> 4. Changed source URL to be more straightforward and used autosetup macro
> accordingly
> 5. Removed the apxs macros as i was not sure whether they are neccessary
> (please return them if they are truly necessary but package builds and
> installs fine without them)
> 6. Defined the aplipdir on top of the spec so it can be defined just once
> 7. Updated build section so it uses macros for configure script and parallel
> build
> 8. Removed %defattr(0644,root,root,0755) as it is not necessary
> 9. Added %license record to %files section
> 10. Updated License tag so it conforms to SPDX naming
> 11. Added requirement of http_port_t type to your selinux policy
> 
> Now i have a few questions. On github i see that the latest version is 2.0.3.
> Is there a reason why the package ships 1.3.19?
mod_proxy_cluster 1.3.19 can work with http 2.4.57 well (httpd version is 2.4.57 in fedora), but we haven't done the fully test for mod_proxy_cluster 2.0.3 with httpd 2.4.57. So I think importing mod_proxy_cluster is safe, I will create a bugzilla for upgrading it to the latest version.
> Also i see that there are excluded archs i686 i386. Is it necessary?
Yes, we haven't support them for some years.
> If it truly is then you will have to file bugs for both of them after the
> release
> and explain why is it so.
> 
> Looking forward to getting your opinion.

Comment 5 huwang 2023-09-30 12:17:17 UTC
The scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106912067

Comment 6 Tomas Korbar 2023-10-02 09:17:34 UTC
Hi, thanks for the changes. I am ok with the package now.
Giving you fedora-review +.

Comment 7 Fedora Admin user for bugzilla script actions 2023-10-11 06:20:29 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/mod_proxy_cluster


Note You need to log in before you can comment on or make changes to this bug.