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

feat: add apisix fedora support #110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: add apisix fedora support #110

wants to merge 2 commits into from

Conversation

Kushal-kothari
Copy link
Contributor

No description provided.

@membphis membphis requested a review from imjoey October 11, 2021 10:28
@membphis
Copy link
Contributor

hi, do you have time to look at this PR? @imjoey

Copy link
Contributor

@imjoey imjoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kushal-kothari Hi, great job and really impressive. I would prefer to reuse current code as much as possible in this PR, cause maybe some other distributions, such as RockyLinux, will be supported some day. Please let me know what do you think. Thanks.

@@ -179,8 +194,8 @@ build-fpm:
-t api7/fpm - < ./dockerfiles/Dockerfile.fpm
endif

ifeq ($(filter $(app),apisix dashboard apisix-base),)
$(info the app's value have to be apisix or dashboard!)
ifeq ($(filter $(app),apisix_fedora apisix dashboard apisix-base),)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kushal-kothari there's no need to add this cause we still use apisix here.

Comment on lines +30 to +31
make package type=rpm app=apisix_fedora version=master checkout=master image_base=fedora image_tag=34 artifact=apisix-remote
make package type=rpm app=apisix_fedora version=2.2 checkout=2.2 image_base=fedora image_tag=34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
make package type=rpm app=apisix_fedora version=master checkout=master image_base=fedora image_tag=34 artifact=apisix-remote
make package type=rpm app=apisix_fedora version=2.2 checkout=2.2 image_base=fedora image_tag=34
make package type=rpm app=apisix version=master checkout=master image_base=fedora image_tag=34 artifact=apisix-remote
make package type=rpm app=apisix version=2.2 checkout=2.2 image_base=fedora image_tag=34

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kushal-kothari we would better to keep using apisix here.

COPY package-apisix.sh /package-apisix.sh
COPY usr /usr

RUN /package-apisix.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An extra new line is needed here. Thanks.

@@ -6,9 +6,11 @@ dist="el7"
if [ "${IMAGE_BASE}" == "centos" ]
then
dist="el${IMAGE_TAG}"
elif [ "${IMAGE_BASE}" == "fedora" ]
then
dist="${IMAGE_BASE}${IMAGE_TAG}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the rpm in Fedora 34, the conventional dist is fc34, instead of fedora34. Thanks.

# install apisix
RUN /install-common.sh install_apisix \
# determine dist and write it into /tmp/dist file
&& /determine-dist.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would better add an extra line here. Thanks.

Comment on lines +36 to +37
name: apisix-2.2-0.fedora34.x86_64.rpm
path: output/apisix-2.2-0.fedora34.x86_64.rpm
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: apisix-2.2-0.fedora34.x86_64.rpm
path: output/apisix-2.2-0.fedora34.x86_64.rpm
name: apisix-2.2-0.fc34.x86_64.rpm
path: output/apisix-2.2-0.fc34.x86_64.rpm

Comment on lines +101 to +109
build-apisix_fedora-rpm:
ifeq ($(local_code_path), 0)
git clone -b $(checkout) $(apisix_repo) ./apisix
$(call build,apisix,apisix_fedora,rpm,"./apisix")
rm -fr ./apisix
else
$(call build,apisix,apisix_fedora,rpm,$(local_code_path))
endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kushal-kothari how about reusing build-apisix-rpm here, and check if the image_base is fedora to decide where to go then?

@@ -0,0 +1,30 @@
ARG IMAGE_BASE="fedora"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how about reusing Dockerfile.apisix.rpm here, without adding this new file? Thanks.


install_fedora_dependencies_rpm() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, we can check if the environment key IMAGE_BASE is fedora, then we could know which function we should call. What do you think?

@Kushal-kothari
Copy link
Contributor Author

Kushal-kothari commented Oct 11, 2021

Thank you so much @imjoey for all the above suggestions.

I would prefer to reuse current code as much as possible in this PR,

True and most of the above changes you suggested are also based on that. I'll fix all the above changes shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants