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

Various fixes #267

Merged
merged 4 commits into from
Apr 8, 2025
Merged

Various fixes #267

merged 4 commits into from
Apr 8, 2025

Conversation

FaramosCZ
Copy link
Contributor

@FaramosCZ FaramosCZ commented Apr 1, 2025

No description provided.

Copy link

github-actions bot commented Apr 1, 2025

Pull Request validation

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by a member
🟢 Approval - Changes were approved


Auto Merge

Failed

🔴 Pull Request has unsupported target branch master, expected branches are: ''

Success

🟢 Pull Request is not marked as draft and it's not blocked by dont-merge label
🟢 Pull Request meet requirements, title has correct form
🟢 Pull Request meet requirements, mergeable is true
🟢 Pull Request meet requirements, mergeable_state is clean

@FaramosCZ FaramosCZ marked this pull request as ready for review April 1, 2025 13:12
@FaramosCZ FaramosCZ marked this pull request as draft April 1, 2025 13:13
@FaramosCZ
Copy link
Contributor Author

[test-all]

Copy link

github-actions bot commented Apr 1, 2025

Testing Farm results

namecomposearchstatusstarted (UTC)timelogs
CentOS Stream 10 - 10.11CentOS-Stream-10x86_64✅ passed03.04.2025 12:34:116min 42stest pipeline
CentOS Stream 9 - 10.5CentOS-Stream-9x86_64✅ passed03.04.2025 12:34:107min 24stest pipeline
CentOS Stream 9 - 10.11CentOS-Stream-9x86_64✅ passed03.04.2025 12:34:106min 60stest pipeline
Fedora - 10.5Fedora-latestx86_64✅ passed03.04.2025 12:34:119min 17stest pipeline
RHEL10 - PyTest - OpenShift 4 - 10.11RHEL-10-Nightlyx86_64❌ error01.04.2025 14:50:2111min 45stest pipeline
RHEL10 - 10.11RHEL-10-Nightlyx86_64✅ passed03.04.2025 12:34:0917min 10stest pipeline
RHEL9 - 10.5RHEL-9.4.0-Nightlyx86_64✅ passed03.04.2025 12:34:1417min 25stest pipeline
RHEL9 - 10.11RHEL-9.4.0-Nightlyx86_64✅ passed03.04.2025 12:34:0918min 35stest pipeline
RHEL8 - 10.5RHEL-8.10.0-Nightlyx86_64✅ passed03.04.2025 12:34:1121min 17stest pipeline
RHEL9 - OpenShift 4 - 10.11RHEL-9.4.0-Nightlyx86_64✅ passed01.04.2025 14:50:3018min 34stest pipeline
RHEL9 - OpenShift 4 - 10.5RHEL-9.4.0-Nightlyx86_64✅ passed01.04.2025 14:50:4118min 32stest pipeline
RHEL10 - OpenShift 4 - 10.11RHEL-10-Nightlyx86_64❌ error01.04.2025 14:58:0111min 5stest pipeline
RHEL8 - 10.11RHEL-8.10.0-Nightlyx86_64✅ passed03.04.2025 12:34:1021min 44stest pipeline
RHEL8 - 10.3RHEL-8.10.0-Nightlyx86_64✅ passed03.04.2025 12:34:1023min 25stest pipeline
RHEL8 - OpenShift 4 - 10.3RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:50:2020min 60stest pipeline
RHEL8 - OpenShift 4 - 10.11RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:50:4021min 57stest pipeline
RHEL8 - OpenShift 4 - 10.5RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:57:4220min 2stest pipeline
RHEL9 - PyTest - OpenShift 4 - 10.11RHEL-9.4.0-Nightlyx86_64✅ passed01.04.2025 14:50:2453min 24stest pipeline
RHEL8 - PyTest - OpenShift 4 - 10.5RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:50:2153min 26stest pipeline
RHEL9 - PyTest - OpenShift 4 - 10.5RHEL-9.4.0-Nightlyx86_64❌ error01.04.2025 14:50:1954min 42stest pipeline
RHEL8 - PyTest - OpenShift 4 - 10.11RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:50:2356min 38stest pipeline
RHEL8 - PyTest - OpenShift 4 - 10.3RHEL-8.10.0-Nightlyx86_64✅ passed01.04.2025 14:50:351h 6min 26stest pipeline

@FaramosCZ
Copy link
Contributor Author

[test]

@FaramosCZ
Copy link
Contributor Author

[test]

@FaramosCZ
Copy link
Contributor Author

[test]

@FaramosCZ
Copy link
Contributor Author

[test]

@FaramosCZ FaramosCZ marked this pull request as ready for review April 2, 2025 10:26
@FaramosCZ
Copy link
Contributor Author

Hi @phracek, this MR is ready for review.

After some experimentation, I only included non-intrusive changes into this MR.
I advise doing the code review commit by commit, as that should be quite readable, unlike the squashed blob.

We discussed applying the same changes to other containerfiles too.
Once you review what I currently have, I can extend the changes to the other files you select.

As I changed the metadata (started using variables there), do we have any elegant way to diff the metadata of released image and one with these changes ?

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. See my comment.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

Can you please make this change also for 10.11/Dockerfile.rhel9, 10.5/Dockerfile.rhel9, and the others so we have consistent naming in the whole mariadb-container?

@FaramosCZ
Copy link
Contributor Author

Can you please make this change also for 10.11/Dockerfile.rhel9, 10.5/Dockerfile.rhel9, and the others so we have consistent naming in the whole mariadb-container?

Sure.

I extended the change to the all relevant 10.11 containerfiles.

  • applying to: Fedora, RHEL 10, C10S, RHEL 9, C9S
  • omitting: RHEL 8, C8S

Do you really wish to extend this change to the 10.5 images too?
If yes, which variants specifically?Just the RHEL 9 + C9S ones ?


!! WARNING !!

  • this introduces the ${NAME} environmental variable to the RHEL 9 and C9S variants !

    • I find this useful
  • this introduces the ${VERSION} environmental variable to the RHEL 9 and C9S variants !

    • I'm not sure if I find this useful. I added it to keep the containerfiles as similar as possible.

But this is a topic for discussion.

@FaramosCZ
Copy link
Contributor Author

[test]

…dibility,

so they are easier to spot in the text thanks to the clear variable name boundaries
…erfile

WARNING:
  this introduces the ${NAME} environmental variable to the RHEL 9 and C9S variants !
…work

- utilize the ${MYSQL_VERSION} variable through the containerfile
- replace all ${VERSION} with ${MYSQL_VERSION}
- introduce ${MYSQL_SHORT_VERSION} environment variable
- utilize the ${MYSQL_SHORT_VERSION} variable through the containerfile
- move all DB version variables together

Removal of seemingly redundant ${VERSION} variable was discussed.
Great caution should be employed when removing existing environment variables,
as users can depend on them.
${VERSION} is currently kept for the compatibility purposes

Change of ${MYSQL_VERSION} to ${MARIADB_VERSION} variable was discussed.
Convincing positives not found at this moment. Might reconsider later.
- note: ${MYSQL_VERSION} is also used in scripts outside of the containerfile
- note: in case of removal of ${MYSQL_VERSION}, caution is advised as described above.

WARNING:
  this introduces the ${VERSION} environmental variable to the RHEL 9 and C9S variants !
@FaramosCZ
Copy link
Contributor Author

I checked that the 10.5 container-files do not contain any new caveats, and extended the change to them, specifically:

  • applying to 10.5 : RHEL 9, C9S

@FaramosCZ
Copy link
Contributor Author

[test]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this issue. LGTM.

@phracek phracek merged commit 84943a1 into sclorg:master Apr 8, 2025
11 checks passed
@FaramosCZ FaramosCZ deleted the various_fixes branch April 8, 2025 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants