This pull request enables dist-gen for mariadb-container - #331
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds ChangesMariaDB container generation and version-specific image updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Pull Request validationFailed🔴 Failed or pending statuses:
🔴 Review - Missing review from a member (1 required) Triggered by Workflow Run |
What was changed in Dockerfiles.
- SUMMARY uses now version instead of ${MYSQL_VERSION}
- io.k8s. uses hardcoded version as well.
- STI_SCRIPTS_PATH is now unified.
- COPY now uses hardcoded version.
New files and directories are:
- src
- specs
- manifest.yml
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
support it at all Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Member
Author
|
[test] |
phracek
requested review from
frenzymadness and
pkhartsk
and removed request for
pkhartsk
June 9, 2026 13:35
PavolSloboda
self-requested a review
June 10, 2026 08:24
frenzymadness
left a comment
frenzymadness
left a comment
Member
There was a problem hiding this comment.
My honest opinion is that a developer should review AI-generated code before opening a PR and requesting review from others, and I have a feeling that it did not happen here.
See the inline comments.
And one more thing: we should document the new workflow for this repository in the readme.
Comment on lines
+58
to
+60
| # 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. |
Member
There was a problem hiding this comment.
How is this related to the packages installation below?
Member
Author
There was a problem hiding this comment.
Original is here w/o distgen https://github.com/sclorg/mariadb-container/blob/master/10.11/Dockerfile.rhel10#L41
Member
There was a problem hiding this comment.
So if it was wrong before, the goal is to still keep it wrong.
Member
Author
There was a problem hiding this comment.
Let's remove it. IT really does not make sense
FaramosCZ
reviewed
Jun 10, 2026
Member
+1, but was this AI-assisted though? I do not see it being marked as such in the commit. |
Fix also update_doc_url to closes as possible to version Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Use only 'dnf' command. Use spec.com_redhat_component Fix usage generation Simplify condition for INSTALL_PKGS and enabling module. Remove obsoleted empty lines that does not make sense Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Use specific spec directives Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Remove version label. Let's make the condition for installing package more easier. Fix README.md file generated by dist-gen as well. Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
It does not have any effect Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
10.11/Dockerfile.fedora (1)
59-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent version path handling across 10.11 Dockerfiles.
Lines 59-61 use the
${MYSQL_VERSION}variable inCOPYcommands, while all other 10.11 Dockerfiles (rhel8, rhel9, rhel10, c10s, c9s) hardcode the path as10.11/. This inconsistency contradicts the PR objective of hardcoding version-specific paths across all variants.🔧 Proposed fix to align with other 10.11 Dockerfiles
-COPY ${MYSQL_VERSION}/root-common / -COPY ${MYSQL_VERSION}/s2i-common/bin/ ${STI_SCRIPTS_PATH} -COPY ${MYSQL_VERSION}/root / +COPY 10.11/root-common / +COPY 10.11/s2i-common/bin/ $STI_SCRIPTS_PATH +COPY 10.11/root /Note: Also updated
${STI_SCRIPTS_PATH}to$STI_SCRIPTS_PATHto match the variable interpolation style used in other 10.11 Dockerfiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.fedora` around lines 59 - 61, The COPY commands use the variable ${MYSQL_VERSION} and ${STI_SCRIPTS_PATH}, which is inconsistent with other 10.11 Dockerfiles that hardcode the version; update the three COPY lines to use the hardcoded path prefix "10.11/" instead of ${MYSQL_VERSION} and normalize the script target variable to $STI_SCRIPTS_PATH (replace ${STI_SCRIPTS_PATH} with $STI_SCRIPTS_PATH) so the COPY invocations match the other 10.11 variants.11.8/Dockerfile.fedora (1)
59-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent path hardcoding: COPY still uses
${MYSQL_VERSION}variable.The other four 11.8 Dockerfiles in this layer (rhel9, rhel10, c10s, c9s) hardcode their COPY paths to
11.8/, but this Fedora variant still uses the${MYSQL_VERSION}variable. While functionally equivalent sinceMYSQL_VERSION=11.8, this creates inconsistency within the layer. The layer description states artifact paths should be updated from${MYSQL_VERSION}/to11.8/*.Similarly, the labels on lines 32-38 remain parameterized (using
${MYSQL_VERSION},${NAME},${MYSQL_SHORT_VERSION}) rather than hardcoded values like the other variants.🔧 Proposed fix to hardcode COPY paths
-COPY ${MYSQL_VERSION}/root-common / -COPY ${MYSQL_VERSION}/s2i-common/bin/ ${STI_SCRIPTS_PATH} -COPY ${MYSQL_VERSION}/root / +COPY 11.8/root-common / +COPY 11.8/s2i-common/bin/ $STI_SCRIPTS_PATH +COPY 11.8/root /🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@11.8/Dockerfile.fedora` around lines 59 - 61, Replace the parameterized paths and labels in the Fedora Dockerfile with the same hardcoded values used by the other 11.8 variants: change the three COPY lines that reference ${MYSQL_VERSION} (COPY ${MYSQL_VERSION}/root-common, COPY ${MYSQL_VERSION}/s2i-common/bin/, COPY ${MYSQL_VERSION}/root) to use the literal prefix "11.8/" and update the Dockerfile LABEL entries (the labels around lines 32–38 that currently use ${MYSQL_VERSION}, ${NAME}, ${MYSQL_SHORT_VERSION}) to their hardcoded equivalents (e.g., set NAME, MYSQL_VERSION, MYSQL_SHORT_VERSION to the 11.8-specific string values used in the other layer files) so the Fedora Dockerfile matches the layer’s non-parameterized convention.
♻️ Duplicate comments (1)
specs/multispec.yml (1)
111-111:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix copy-paste error in upgrade_skip_note.
The
upgrade_skip_notesays "from MariaDB 10.3 to 11.11" but this metadata is for version 11.8, not 11.11. This appears to be a copy-paste error from the 10.11 version metadata.📝 Proposed fix
- upgrade_skip_note: "MariaDB 5.5 to MariaDB 10.0 and from MariaDB 10.3 to 11.11." + upgrade_skip_note: "MariaDB 5.5 to MariaDB 10.0 and from MariaDB 10.3 to 11.8."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/multispec.yml` at line 111, The upgrade_skip_note contains a copy-paste error referencing "11.11" instead of this spec's target version; update the string value of the upgrade_skip_note key to read "MariaDB 5.5 to MariaDB 10.0 and from MariaDB 10.3 to 11.8." so the metadata correctly reflects version 11.8 (locate the upgrade_skip_note entry in specs/multispec.yml and change the trailing version number from 11.11 to 11.8).
🧹 Nitpick comments (10)
10.5/Dockerfile.rhel8 (1)
28-28: ⚡ Quick winMisleading comment appears in all three 10.5 Dockerfiles.
The comment "Standalone ENV call so these values can be re-used in the other ENV calls" incorrectly describes the LABEL block that follows it. This appears to be a copy-paste error from the ENV comment above. The comment should describe that the LABEL block references the environment variables defined earlier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.5/Dockerfile.rhel8` at line 28, The comment above the LABEL block is incorrect; update the comment that currently reads "Standalone ENV call so these values can be re-used in the other ENV calls" to indicate that the LABEL block references the environment variables defined earlier (i.e., explain that LABEL populates image metadata from the previously defined ENV variables), so replace the misleading text with a short note that the LABEL entries consume the earlier ENV values.10.11/Dockerfile.rhel10 (1)
28-28: ⚡ Quick winCorrect the misleading comment.
The comment on line 28 states "Standalone ENV call" but the following lines 29-39 are a
LABELblock, not anENVblock.♻️ Proposed fix
-# Standalone ENV call so these values can be re-used in the other ENV calls +# Label block re-uses ENV variables defined above LABEL summary="${SUMMARY}" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.rhel10` at line 28, Update the misleading comment that currently reads "Standalone ENV call" to accurately describe the following block as a LABEL block (e.g., "Standalone LABEL block so these values can be re-used in other LABEL instructions" or similar) and ensure it sits immediately above the LABEL block referenced in the diff so readers know the metadata labels (lines 29-39 in context) are being referenced, not environment variables.10.11/Dockerfile.rhel8 (2)
46-46: ⚡ Quick winUse
dnfconsistently for all package management operations.Line 46 uses
yumto enable the MariaDB module, but lines 48 and 51 usednffor package installation and cleanup. For consistency and to use the modern package manager throughout, replaceyumwithdnfon line 46.♻️ Proposed fix
-RUN yum -y module enable mariadb:10.11 && \ +RUN dnf -y module enable mariadb:10.11 && \ INSTALL_PKGS="policycoreutils rsync tar gettext hostname groff-base ${NAME}-server" && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.rhel8` at line 46, Replace the inconsistent package manager call in the Dockerfile RUN line that currently uses "yum -y module enable mariadb:10.11" with "dnf -y module enable mariadb:10.11" so the module enable step matches the other package operations (see the RUN line containing "module enable mariadb:10.11"); ensure the switch keeps the "-y" flag and placement in the multi-line RUN chain unchanged.
28-28: ⚡ Quick winCorrect the misleading comment.
The comment on line 28 states "Standalone ENV call" but the following lines 29-39 are a
LABELblock, not anENVblock.♻️ Proposed fix
-# Standalone ENV call so these values can be re-used in the other ENV calls +# Label block re-uses ENV variables defined above LABEL summary="${SUMMARY}" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.rhel8` at line 28, The comment incorrectly calls the following block a "Standalone ENV call" but the subsequent block is a LABEL metadata block; update the comment to accurately describe the block (e.g., "Standalone LABEL block so these values can be re-used in other instructions" or similar) so it references the LABEL entries that follow (the LABEL block spanning the lines after the comment).10.11/Dockerfile.c10s (1)
28-28: ⚡ Quick winCorrect the misleading comment.
The comment on line 28 states "Standalone ENV call" but the following lines 29-39 are a
LABELblock, not anENVblock.♻️ Proposed fix
-# Standalone ENV call so these values can be re-used in the other ENV calls +# Label block re-uses ENV variables defined above LABEL summary="${SUMMARY}" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.c10s` at line 28, The comment is misleading: it says "Standalone ENV call" but the following block is a LABEL block; update the comment above the LABEL block (the comment on line 28) to accurately describe the LABEL block (e.g., "Standalone LABEL block so these values can be re-used") to match the LABEL lines 29-39 and avoid confusion when editing Dockerfile.c10s.10.11/Dockerfile.c9s (2)
28-28: ⚡ Quick winCorrect the misleading comment.
The comment on line 28 states "Standalone ENV call" but the following lines 29-39 are a
LABELblock, not anENVblock.♻️ Proposed fix
-# Standalone ENV call so these values can be re-used in the other ENV calls +# Label block re-uses ENV variables defined above LABEL summary="${SUMMARY}" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.c9s` at line 28, The inline comment currently reads "Standalone ENV call" but sits immediately above a LABEL block (the Dockerfile LABEL ... block), which is misleading; update the comment to accurately describe the following block (e.g., "Standalone LABEL block so these values can be re-used in other instructions" or similar) so it correctly references the LABEL block rather than ENV.
46-46: ⚡ Quick winUse
dnfconsistently for all package management operations.Line 46 uses
yumto enable the MariaDB module, but lines 48 and 51 usednffor package installation and cleanup. For consistency and to use the modern package manager throughout, replaceyumwithdnfon line 46.♻️ Proposed fix
-RUN yum -y module enable mariadb:10.11 && \ +RUN dnf -y module enable mariadb:10.11 && \ INSTALL_PKGS="policycoreutils rsync tar gettext hostname groff-base ${NAME}-server" && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.c9s` at line 46, Replace the use of yum with dnf in the Dockerfile RUN that enables the MariaDB module: change the command that currently calls "yum -y module enable mariadb:10.11" to use "dnf -y module enable mariadb:10.11" so package management is consistent with the later "dnf" package install and cleanup steps in the same RUN sequence.10.11/Dockerfile.rhel9 (1)
28-28: ⚡ Quick winCorrect the misleading comment.
The comment on line 28 states "Standalone ENV call" but the following lines 29-39 are a
LABELblock, not anENVblock.♻️ Proposed fix
-# Standalone ENV call so these values can be re-used in the other ENV calls +# Label block re-uses ENV variables defined above LABEL summary="${SUMMARY}" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.11/Dockerfile.rhel9` at line 28, Update the misleading comment that reads "Standalone ENV call so these values can be re-used in the other ENV calls" to correctly reference the LABEL block that follows; replace it with a concise comment such as "Standalone LABEL block so these values can be re-used in the other LABEL entries" (or similar) to accurately describe the subsequent LABEL block spanning the LABEL directives.src/Dockerfile.fedora (1)
48-48: ⚡ Quick winConsider using
{{ spec.pkgs }}for consistency.Line 48 hardcodes the package list, but
specs/multispec.ymlline 15 definespkgsfor fedora42. For consistency with the template approach used insrc/Dockerfile, consider using the spec variable:- INSTALL_PKGS="policycoreutils rsync tar gettext hostname groff-base" && \ + INSTALL_PKGS="{{ spec.pkgs }}" && \This would make package list changes easier to maintain centrally in multispec.yml.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Dockerfile.fedora` at line 48, The Dockerfile.fedora currently hardcodes INSTALL_PKGS ("policycoreutils rsync tar gettext hostname groff-base"); replace this literal with the spec variable used by the template system (referencing spec.pkgs from specs/multispec.yml) so the Fedora build uses {{ spec.pkgs }} like src/Dockerfile does; update the INSTALL_PKGS assignment in Dockerfile.fedora to reference the spec variable (INSTALL_PKGS="{{ spec.pkgs }}" or equivalent per template syntax) ensuring packaging is maintained centrally.10.3/Dockerfile.rhel8 (1)
46-48: ⚡ Quick winUse consistent package manager (dnf instead of yum).
Line 46 uses
yumto enable the module, while line 48 usesdnfto install packages. Althoughyumis a symlink todnfin RHEL8, usingdnfconsistently improves clarity and aligns with modern RHEL practices.♻️ Proposed fix to use dnf consistently
-RUN yum -y module enable mariadb:10.3 && \ +RUN dnf -y module enable mariadb:10.3 && \ INSTALL_PKGS="policycoreutils rsync tar gettext hostname groff-base ${NAME}-server" && \ dnf install -y --setopt=tsflags=nodocs ${INSTALL_PKGS} && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@10.3/Dockerfile.rhel8` around lines 46 - 48, Replace the mixed use of yum and dnf by using dnf for enabling the MariaDB module; specifically change the command that calls "yum -y module enable mariadb:10.3" to use "dnf -y module enable mariadb:10.3" so the module enablement is consistent with the subsequent dnf install that uses the INSTALL_PKGS variable and avoids mixed package manager usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@10.3/root/usr/share/container-scripts/mysql/README.md`:
- Line 299: Fix the spelling mistake in the README line that currently reads
"the only exception is ugrading from MariaDB 5.5 to MariaDB 10.0 and from
MariaDB 10.3 to 10.11." by changing "ugrading" to "upgrading" so the sentence
reads "the only exception is upgrading from MariaDB 5.5 to MariaDB 10.0 and from
MariaDB 10.3 to 10.11."
- Around line 365-370: The README.md "See also" section in
10.3/root/usr/share/container-scripts/mysql/README.md references
Dockerfile.rhel10 and Dockerfile.c10s which do not exist in the 10.3/ tree;
either remove those references or add the missing Dockerfiles. Locate the "See
also" text lines (mentions of Dockerfile.rhel10 and Dockerfile.c10s) and update
them to only list Dockerfile.rhel8, Dockerfile.c9s, Dockerfile.fedora (or add
Dockerfile.rhel10/Dockerfile.c10s under 10.3 if you intend to support them),
ensuring the README entries accurately reflect the files present for MariaDB
10.3.
- Around line 296-299: Update the README text that mentions "Skipping versions
like from X.Y to X.Y+2 or downgrading..." and the "10.3 to 10.11" exception by
adding an explicit reference/link to MariaDB's official guidance for upgrades
between major versions (suggested URL:
https://mariadb.com/docs/server/server-management/install-and-upgrade-mariadb/upgrading/platform-specific-upgrade-guides/upgrading-on-linux/upgrading-between-major-mariadb-versions.md)
immediately after the "10.3 to 10.11" sentence so readers can find authoritative
details about that specific exception; ensure the new sentence clearly cites the
10.3→10.11 exception and includes the URL.
In `@10.5/Dockerfile.rhel8`:
- Line 46: Replace the yum call in the Dockerfile RUN line that enables the
MariaDB module with dnf to match the "use dnf exclusively" objective: update the
command using the RUN instruction that currently invokes "yum -y module enable
mariadb:10.5" to use "dnf -y module enable mariadb:10.5" so it is consistent
with the later "dnf" package installation steps.
In `@10.5/root/usr/share/container-scripts/mysql/README.md`:
- Around line 296-300: In the README.md upgrade note, correct the typo
"ugrading" to "upgrading" in the sentence discussing exceptions (the line that
reads "the only exception is ugrading from MariaDB 5.5..."); update that word to
"upgrading" so the sentence reads "the only exception is upgrading from MariaDB
5.5..." and ensure no other spelling errors remain in that paragraph.
In `@11.8/Dockerfile.c9s`:
- Line 46: Replace the use of yum in the RUN that enables the MariaDB module
with dnf to be consistent with the other package commands: change the command
that currently calls "yum -y module enable mariadb:11.8" so it uses "dnf -y
module enable mariadb:11.8" (preserving the rest of the RUN chain and flags),
ensuring the module enable step in the Dockerfile.c9s aligns with the later dnf
install and dnf clean commands.
In `@src/Dockerfile`:
- Line 69: The Dockerfile templates use the undefined environment variable
STI_SCRIPTS_PATH in COPY directives (COPY ... $STI_SCRIPTS_PATH /
${STI_SCRIPTS_PATH}) which can fail; add an explicit ENV
STI_SCRIPTS_PATH=/usr/local/s2i/bin (or another intended path) near the top of
both Dockerfile templates before any COPY that references it, or alternatively
verify that the S2I base image defines STI_SCRIPTS_PATH and document that
dependency; update the Dockerfile(s) to set ENV STI_SCRIPTS_PATH if not present
so the COPY commands (the lines using COPY {{ spec.version }}/s2i-common/bin/
$STI_SCRIPTS_PATH and similar) reliably target the correct destination.
- Around line 47-57: The Dockerfile template mishandles module enablement for
spec.version == "10.5": the c9s branch currently runs "dnf -y module disable
mariadb" but should invoke spec.environment_setup (which enables mariadb:{{
spec.version }}), and the branch for spec.version == "10.5" and spec.prod not in
["c9s","rhel8"] (rhel9) must also run spec.environment_setup before setting
INSTALL_PKGS; update the conditional blocks around spec.version == "10.5" to
call {{ spec.environment_setup }} (instead of disabling the module) in the c9s
case and add {{ spec.environment_setup }} to the rhel9 branch so mariadb module
enablement is performed when creating the image.
In `@src/root/usr/share/container-scripts/mysql/README.md`:
- Line 356: Fix the spelling mistake by replacing "deamon" with "daemon" in the
README sentence that currently reads "The mysqld deamon in the container logs to
the standard output, so the log is available in the container log. The log can
be examined by running:"; update that exact phrase to "The mysqld daemon..." to
correct the typo in the documentation.
- Line 299: Fix the README typo and make the upgrade-exception version-specific:
correct “ugrading” to “upgrading” and replace the hardcoded exception text (“the
only exception is upgrading from MariaDB 5.5 to MariaDB 10.0 and from MariaDB
10.3 to 10.11.”) with the template variable {{ spec.upgrade_skip_note }} so the
README uses the per-version notes defined in specs/multispec.yml; update the
sentence near the existing upgrade note to reference spec.upgrade_skip_note
instead of the static version list.
---
Outside diff comments:
In `@10.11/Dockerfile.fedora`:
- Around line 59-61: The COPY commands use the variable ${MYSQL_VERSION} and
${STI_SCRIPTS_PATH}, which is inconsistent with other 10.11 Dockerfiles that
hardcode the version; update the three COPY lines to use the hardcoded path
prefix "10.11/" instead of ${MYSQL_VERSION} and normalize the script target
variable to $STI_SCRIPTS_PATH (replace ${STI_SCRIPTS_PATH} with
$STI_SCRIPTS_PATH) so the COPY invocations match the other 10.11 variants.
In `@11.8/Dockerfile.fedora`:
- Around line 59-61: Replace the parameterized paths and labels in the Fedora
Dockerfile with the same hardcoded values used by the other 11.8 variants:
change the three COPY lines that reference ${MYSQL_VERSION} (COPY
${MYSQL_VERSION}/root-common, COPY ${MYSQL_VERSION}/s2i-common/bin/, COPY
${MYSQL_VERSION}/root) to use the literal prefix "11.8/" and update the
Dockerfile LABEL entries (the labels around lines 32–38 that currently use
${MYSQL_VERSION}, ${NAME}, ${MYSQL_SHORT_VERSION}) to their hardcoded
equivalents (e.g., set NAME, MYSQL_VERSION, MYSQL_SHORT_VERSION to the
11.8-specific string values used in the other layer files) so the Fedora
Dockerfile matches the layer’s non-parameterized convention.
---
Duplicate comments:
In `@specs/multispec.yml`:
- Line 111: The upgrade_skip_note contains a copy-paste error referencing
"11.11" instead of this spec's target version; update the string value of the
upgrade_skip_note key to read "MariaDB 5.5 to MariaDB 10.0 and from MariaDB 10.3
to 11.8." so the metadata correctly reflects version 11.8 (locate the
upgrade_skip_note entry in specs/multispec.yml and change the trailing version
number from 11.11 to 11.8).
---
Nitpick comments:
In `@10.11/Dockerfile.c10s`:
- Line 28: The comment is misleading: it says "Standalone ENV call" but the
following block is a LABEL block; update the comment above the LABEL block (the
comment on line 28) to accurately describe the LABEL block (e.g., "Standalone
LABEL block so these values can be re-used") to match the LABEL lines 29-39 and
avoid confusion when editing Dockerfile.c10s.
In `@10.11/Dockerfile.c9s`:
- Line 28: The inline comment currently reads "Standalone ENV call" but sits
immediately above a LABEL block (the Dockerfile LABEL ... block), which is
misleading; update the comment to accurately describe the following block (e.g.,
"Standalone LABEL block so these values can be re-used in other instructions" or
similar) so it correctly references the LABEL block rather than ENV.
- Line 46: Replace the use of yum with dnf in the Dockerfile RUN that enables
the MariaDB module: change the command that currently calls "yum -y module
enable mariadb:10.11" to use "dnf -y module enable mariadb:10.11" so package
management is consistent with the later "dnf" package install and cleanup steps
in the same RUN sequence.
In `@10.11/Dockerfile.rhel10`:
- Line 28: Update the misleading comment that currently reads "Standalone ENV
call" to accurately describe the following block as a LABEL block (e.g.,
"Standalone LABEL block so these values can be re-used in other LABEL
instructions" or similar) and ensure it sits immediately above the LABEL block
referenced in the diff so readers know the metadata labels (lines 29-39 in
context) are being referenced, not environment variables.
In `@10.11/Dockerfile.rhel8`:
- Line 46: Replace the inconsistent package manager call in the Dockerfile RUN
line that currently uses "yum -y module enable mariadb:10.11" with "dnf -y
module enable mariadb:10.11" so the module enable step matches the other package
operations (see the RUN line containing "module enable mariadb:10.11"); ensure
the switch keeps the "-y" flag and placement in the multi-line RUN chain
unchanged.
- Line 28: The comment incorrectly calls the following block a "Standalone ENV
call" but the subsequent block is a LABEL metadata block; update the comment to
accurately describe the block (e.g., "Standalone LABEL block so these values can
be re-used in other instructions" or similar) so it references the LABEL entries
that follow (the LABEL block spanning the lines after the comment).
In `@10.11/Dockerfile.rhel9`:
- Line 28: Update the misleading comment that reads "Standalone ENV call so
these values can be re-used in the other ENV calls" to correctly reference the
LABEL block that follows; replace it with a concise comment such as "Standalone
LABEL block so these values can be re-used in the other LABEL entries" (or
similar) to accurately describe the subsequent LABEL block spanning the LABEL
directives.
In `@10.3/Dockerfile.rhel8`:
- Around line 46-48: Replace the mixed use of yum and dnf by using dnf for
enabling the MariaDB module; specifically change the command that calls "yum -y
module enable mariadb:10.3" to use "dnf -y module enable mariadb:10.3" so the
module enablement is consistent with the subsequent dnf install that uses the
INSTALL_PKGS variable and avoids mixed package manager usage.
In `@10.5/Dockerfile.rhel8`:
- Line 28: The comment above the LABEL block is incorrect; update the comment
that currently reads "Standalone ENV call so these values can be re-used in the
other ENV calls" to indicate that the LABEL block references the environment
variables defined earlier (i.e., explain that LABEL populates image metadata
from the previously defined ENV variables), so replace the misleading text with
a short note that the LABEL entries consume the earlier ENV values.
In `@src/Dockerfile.fedora`:
- Line 48: The Dockerfile.fedora currently hardcodes INSTALL_PKGS
("policycoreutils rsync tar gettext hostname groff-base"); replace this literal
with the spec variable used by the template system (referencing spec.pkgs from
specs/multispec.yml) so the Fedora build uses {{ spec.pkgs }} like
src/Dockerfile does; update the INSTALL_PKGS assignment in Dockerfile.fedora to
reference the spec variable (INSTALL_PKGS="{{ spec.pkgs }}" or equivalent per
template syntax) ensuring packaging is maintained centrally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee01e44d-0d29-4d93-92f7-678aa1d94aa1
📒 Files selected for processing (25)
.github/workflows/build-and-push.yml10.11/Dockerfile.c10s10.11/Dockerfile.c9s10.11/Dockerfile.fedora10.11/Dockerfile.rhel1010.11/Dockerfile.rhel810.11/Dockerfile.rhel910.11/root/usr/share/container-scripts/mysql/README.md10.3/Dockerfile.rhel810.3/root/usr/share/container-scripts/mysql/README.md10.5/Dockerfile.c9s10.5/Dockerfile.rhel810.5/Dockerfile.rhel910.5/root/usr/share/container-scripts/mysql/README.md11.8/Dockerfile.c10s11.8/Dockerfile.c9s11.8/Dockerfile.fedora11.8/Dockerfile.rhel1011.8/Dockerfile.rhel911.8/root/usr/share/container-scripts/mysql/README.mdmanifest.ymlspecs/multispec.ymlsrc/Dockerfilesrc/Dockerfile.fedorasrc/root/usr/share/container-scripts/mysql/README.md
💤 Files with no reviewable changes (1)
- .github/workflows/build-and-push.yml
Fix typo in README.md file Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Member
Author
|
@PavolSloboda @frenzymadness I would like to merge it this week. We can address the rest of issue by the other PR in case you are not against it. |
frenzymadness
left a comment
frenzymadness
left a comment
Member
There was a problem hiding this comment.
Provided some more comments.
Comment on lines
+58
to
+60
| # 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. |
Member
There was a problem hiding this comment.
So if it was wrong before, the goal is to still keep it wrong.
| # 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. | ||
| {% if spec.version == "10.3" or (spec.version == "10.5" and spec.prod == "rhel8") or (spec.version in ["10.11", "11.8"] and spec.prod not in ["c10s", "rhel10"]) %} |
Member
There was a problem hiding this comment.
This complex logic would really benefit from a comment above it describing all the possibilities. I cannot really imagine adding a new version into this.
Member
Author
There was a problem hiding this comment.
Sorry I do not know how to do it better.
See different aproach here: https://github.com/sclorg/postgresql-container/blob/master/src/Dockerfile#L56 I dont' know what is better.
Do not mention comment twice
Use {{ spec.version }} accross all Dockerfiles.
Add whole upgrade_skip_note sentence to multispec
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Member
Author
|
[test][test-pytest] |
Testing Farm results
|
frenzymadness
left a comment
frenzymadness
left a comment
Member
There was a problem hiding this comment.
One last nitpick. Feel free to ignore it and merge it as is.
Co-authored-by: Lumír 'Frenzy' Balhar <frenzy.madness@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add dist-gen source files.
What was changed in Dockerfiles.
New files and directories are:
Build-and-push is updated.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation