add distgen support#437
Conversation
|
[test] |
Pull Request validationFailed🔴 Failed or pending statuses:
🔴 Review - Missing review from a member (1 required) Triggered by Workflow Run |
Testing Farm results
|
|
[test] |
|
Would it also make sense to include the |
pkhartsk
left a comment
There was a problem hiding this comment.
Found some possible simplifications of the (IMO) very complicated src/Dockerfile
| {% if spec.prod == 'fedora' %}# This image must forever use UID 27 for mysql user so our volumes are safe in the future. This should *never* change. | ||
| # Instead of relying on the DB server package, we will do the setup ourselves before any package is installed | ||
| {% else %}# This image must forever use UID 27 for mysql user so our volumes are | ||
| # safe in the future. This should *never* change, the last test is there | ||
| # to make sure of that. |
There was a problem hiding this comment.
I'm aware that this would cause the dockerfiles to differ cosmetically, but functionally they remain the same, and this is just pretty much the same comment...
| io.k8s.display-name="MySQL ${MYSQL_VERSION}" \ | ||
| io.openshift.expose-services="3306:mysql" \ | ||
| io.openshift.tags="database,${NAME},${NAME}${MYSQL_SHORT_VERSION}{% if spec.prod == 'fedora' and spec.version == '8.0' %},rh-${NAME}${MYSQL_SHORT_VERSION}{% elif spec.prod != 'fedora' %},${NAME}-${MYSQL_SHORT_VERSION}{% endif %}" \ | ||
| com.redhat.component="{% if spec.prod == 'rhel8' or spec.prod == 'rhel9' or spec.prod == 'rhel10' or spec.prod == 'c9s' or spec.prod == 'c10s' %}${NAME}-${MYSQL_SHORT_VERSION}-container{% else %}${NAME}{% endif %}" \ |
There was a problem hiding this comment.
Can be simplified to if spec.prod != 'fedora'
| com.redhat.component="{% if spec.prod == 'rhel8' or spec.prod == 'rhel9' or spec.prod == 'rhel10' or spec.prod == 'c9s' or spec.prod == 'c10s' %}${NAME}-${MYSQL_SHORT_VERSION}-container{% else %}${NAME}{% endif %}" \ | ||
| name="{{ spec.img_name }}" \ | ||
| {% if spec.prod == 'fedora' %} version="${MYSQL_VERSION}" \ | ||
| {% elif spec.prod != 'rhel10' %} version="1" \ |
There was a problem hiding this comment.
This makes no sense to me, why would the version be 1? Shouldn't it always be MYSQL_VERSION? Guessing this is a mistake in the .rhelx Dockerfiles?
| {% endif %}{% if spec.license_terms %} com.redhat.license_terms="{{ spec.license_terms }}" \ | ||
| {% endif %} usage="{% if spec.prod == 'c9s' or spec.prod == 'c10s' %}podman run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 quay.io/{{ spec.img_name }}:{{ spec.prod }}{% elif spec.prod == 'fedora' %}docker run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 quay.io/{{ spec.img_name }}{% else %}podman run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 {{ spec.img_name }}{% endif %}" \ |
There was a problem hiding this comment.
nit: these endif's at the starts of lines are ugly
| {% elif spec.prod == 'rhel8' and spec.version == '8.0' %}RUN yum -y module enable mysql:$MYSQL_VERSION && \ | ||
| INSTALL_PKGS="{{ spec.pkgs }}" && \ | ||
| yum install -y --setopt=tsflags=nodocs $INSTALL_PKGS && \ | ||
| rpm -V $INSTALL_PKGS && \ | ||
| yum -y clean all --enablerepo='*' && \ | ||
| mkdir -p /var/lib/mysql/data && chown -R mysql.0 /var/lib/mysql && \ | ||
| test "$(id mysql)" = "uid=27(mysql) gid=27(mysql) groups=27(mysql)" | ||
| {% elif (spec.prod == 'rhel9' or spec.prod == 'c9s') and spec.version == '8.4' %}RUN yum -y module enable ${NAME}:${MYSQL_VERSION} && \ | ||
| INSTALL_PKGS="{{ spec.pkgs }}" && \ | ||
| dnf install -y --setopt=tsflags=nodocs ${INSTALL_PKGS} && \ | ||
| rpm -V ${INSTALL_PKGS} && \ | ||
| dnf -y clean all --enablerepo='*' && \ | ||
| mkdir -p ${HOME}/data && chown -R mysql:{{ spec.chown_group }} ${HOME} && \ | ||
| test "$(id mysql)" = "uid=27(mysql) gid=27(mysql) groups=27(mysql)" | ||
| {% elif spec.version == '8.0' %}RUN INSTALL_PKGS="{{ spec.pkgs }}" && \ | ||
| yum install -y --setopt=tsflags=nodocs ${INSTALL_PKGS} && \ | ||
| rpm -V ${INSTALL_PKGS} && \ | ||
| yum -y clean all --enablerepo='*' && \ | ||
| mkdir -p ${HOME}/data && chown -R mysql.0 ${HOME} && \ | ||
| test "$(id mysql)" = "uid=27(mysql) gid=27(mysql) groups=27(mysql)" |
There was a problem hiding this comment.
These modular installs seem very similar, maybe the if can be divided into something like
if spec.prod in ['rhel8', ... 'c9s'] for modules where the module is enabled, and then a more common way to install the packages?
| {% if spec.prod == 'rhel8' and spec.version == '8.0' %}COPY {{ spec.version }}/root-common / | ||
| COPY {{ spec.version }}/s2i-common/bin/ $STI_SCRIPTS_PATH | ||
| COPY {{ spec.version }}/root / | ||
| {% else %}COPY ${MYSQL_VERSION}/root-common / | ||
| COPY ${MYSQL_VERSION}/s2i-common/bin/ ${STI_SCRIPTS_PATH} | ||
| COPY ${MYSQL_VERSION}/root / | ||
| {% endif %} |
There was a problem hiding this comment.
Aren't these the exact same?
| {% if spec.prod == 'rhel8' and spec.version == '8.0' %}RUN ln -s /bin/run-mysqld $STI_SCRIPTS_PATH/run | ||
| {% else %}RUN ln -s /bin/run-mysqld ${STI_SCRIPTS_PATH}/run | ||
| {% endif %} |
| {% if spec.prod == 'rhel8' and spec.version == '8.0' %}RUN rm -rf /etc/my.cnf.d/* && \ | ||
| /usr/libexec/container-setup && \ | ||
| rpm-file-permissions && \ | ||
| /usr/libexec/mysqld -V | grep -qe "$MYSQL_VERSION\." && echo "Found VERSION $MYSQL_VERSION" | ||
| {% else %}RUN rm -rf /etc/my.cnf.d/* && \ | ||
| /usr/libexec/container-setup && \ | ||
| rpm-file-permissions && \ | ||
| /usr/libexec/mysqld -V | grep -qe "${MYSQL_VERSION}\." && echo "Found VERSION ${MYSQL_VERSION}" | ||
| {% endif %} |
| {% if spec.prod == 'rhel8' and spec.version == '8.0' %}ENV MYSQL_VERSION={{ spec.version }} \ | ||
| APP_DATA=/opt/app-root/src \ | ||
| HOME=/var/lib/mysql | ||
|
|
||
| ENV SUMMARY="MySQL {{ spec.version }} SQL database server" \ | ||
| DESCRIPTION="MySQL is a multi-user, multi-threaded SQL database server. The container \ | ||
| image provides a containerized packaging of the MySQL mysqld daemon and client application. \ | ||
| The mysqld server daemon accepts connections from clients and provides access to content from \ | ||
| MySQL databases on behalf of the clients." | ||
|
|
||
| LABEL summary="$SUMMARY" \ | ||
| description="$DESCRIPTION" \ | ||
| io.k8s.description="$DESCRIPTION" \ | ||
| io.k8s.display-name="MySQL {{ spec.version }}" \ | ||
| io.openshift.expose-services="3306:mysql" \ | ||
| io.openshift.tags="database,mysql,mysql{{ spec.short }},mysql-{{ spec.short }}" \ | ||
| com.redhat.component="mysql-{{ spec.short }}-container" \ | ||
| name="{{ spec.img_name }}" \ | ||
| version="1" \ | ||
| {% if spec.license_terms %} com.redhat.license_terms="{{ spec.license_terms }}" \ | ||
| {% endif %} usage="podman run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 {{ spec.img_name }}" \ | ||
| maintainer="SoftwareCollections.org <[email protected]>" | ||
| {% else %}# Standalone ENV call so these values can be re-used in the other ENV calls | ||
| ENV MYSQL_VERSION={{ spec.version }} \ | ||
| MYSQL_SHORT_VERSION={{ spec.short }} | ||
|
|
||
| ENV APP_DATA=/opt/app-root/src \ | ||
| HOME=/var/lib/mysql \ | ||
| NAME=mysql \ | ||
| SUMMARY="MySQL ${MYSQL_VERSION} SQL database server" \ | ||
| DESCRIPTION="MySQL is a multi-user, multi-threaded SQL database server. The container \ | ||
| image provides a containerized packaging of the MySQL mysqld daemon and client application. \ | ||
| The mysqld server daemon accepts connections from clients and provides access to content from \ | ||
| MySQL databases on behalf of the clients." | ||
|
|
||
| LABEL summary="${SUMMARY}" \ | ||
| description="${DESCRIPTION}" \ | ||
| io.k8s.description="${DESCRIPTION}" \ | ||
| io.k8s.display-name="MySQL ${MYSQL_VERSION}" \ | ||
| io.openshift.expose-services="3306:mysql" \ | ||
| io.openshift.tags="database,${NAME},${NAME}${MYSQL_SHORT_VERSION}{% if spec.prod == 'fedora' and spec.version == '8.0' %},rh-${NAME}${MYSQL_SHORT_VERSION}{% elif spec.prod != 'fedora' %},${NAME}-${MYSQL_SHORT_VERSION}{% endif %}" \ | ||
| com.redhat.component="{% if spec.prod == 'rhel8' or spec.prod == 'rhel9' or spec.prod == 'rhel10' or spec.prod == 'c9s' or spec.prod == 'c10s' %}${NAME}-${MYSQL_SHORT_VERSION}-container{% else %}${NAME}{% endif %}" \ | ||
| name="{{ spec.img_name }}" \ | ||
| {% if spec.prod == 'fedora' %} version="${MYSQL_VERSION}" \ | ||
| {% elif spec.prod != 'rhel10' %} version="1" \ | ||
| {% endif %}{% if spec.license_terms %} com.redhat.license_terms="{{ spec.license_terms }}" \ | ||
| {% endif %} usage="{% if spec.prod == 'c9s' or spec.prod == 'c10s' %}podman run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 quay.io/{{ spec.img_name }}:{{ spec.prod }}{% elif spec.prod == 'fedora' %}docker run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 quay.io/{{ spec.img_name }}{% else %}podman run -d -e MYSQL_USER=user -e MYSQL_PASSWORD=pass -e MYSQL_DATABASE=db -p 3306:3306 {{ spec.img_name }}{% endif %}" \ | ||
| maintainer="SoftwareCollections.org <[email protected]>" | ||
| {% endif %} |
There was a problem hiding this comment.
Extremely long if and the only differences I see are the use of {{spec.version}} instead of ${MYSQL_VERSION} and similar small things that evaluate to the same thing during buildtime. Maybe this can be simplified somehow?
ccf4eb6 to
4949016
Compare
I believe this should already be present. |
4949016 to
9283c5c
Compare
|
Warning Review limit reached
More reviews will be available in 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[test] |
No description provided.