Fixes #37606 - Drop old CentOS installation media #10227

ekohl · 2024-06-27T17:06:56Z

The host mirror.centos.org is gone now that CentOS < 9 is EOL. It includes a migration to simplify the name, but doesn't include migrations to remove old entries. That is up to the user because they may have modified them and still use them.

chris1984

@ShimShtein do you want me to add this to my list of reviews to go over as part of the mentorship?

chris1984 · 2024-08-05T15:16:48Z

@ekohl did you want me to look for areas that were missed and test(provide a patch), not sure if you just didn't have time or was there another reason this was in a draft?

ekohl

not sure if you just didn't have time

This, though I think it would be great to do this before Foreman 3.12.

I left a comment with what I at least know as one TODO.

ekohl · 2024-08-05T18:02:02Z

db/seeds.d/100-installation_media.rb

-    },
-    {
-      :name => "CentOS Stream 9 mirror",
+      :name => "CentOS mirror",


I think this may need a DB migration to rename the existing entry.

Yup, this will definitely require a migration.

On fresh dev machine:

[2] pry(main)> Medium.where("name like '%9%'").all => [#<Medium:0x00007f08ff4f6aa0 id: 3, name: "CentOS Stream 9 mirror", path: "http://mirror.stream.centos.org/$major-stream/BaseOS/$arch/os", created_at: Tue, 30 Jul 2024 10:43:46.058948000 UTC +00:00, updated_at: Tue, 30 Jul 2024 10:43:46.058948000 UTC +00:00, media_path: nil, config_path: nil, image_path: nil, os_family: "Redhat">]

Should we also delete the Centos Stream mirror and use that in place of the Centos one we are switching to? I see Stream 9 and Stream are both in there?

2 CentOS Stream http://mirror.centos.org/centos/$major-stream/BaseOS/$arch/os 2024-07-19 15:57:37.289 3 CentOS Stream 9 mirror http://mirror.stream.centos.org/$major-stream/BaseOS/$arch/os 2024-07-19 15:57:37.314

Deletion is something we usually don't do. Users may have modified them to point to some internal link. That's a can of worms we prefer to keep closed

chris1984

@ekohl should I update these as well or just keep them since they just relate to tests:

foreman/test/factories/medium.rb

Line 11 in e80d555

path { 'http://mirror.centos.org/centos/$major/os/x86_64' }

I looked through everything in VSCode and don't see anything except for test fixtures, so will make the migration and push it to your branch and wait to hear back from you on the test fixtures and tests.

chris1984

I tried pushing to your branch to add the migration and I am getting this:

[vagrant@toledodevel foreman]$ git push ewoud 37606-drop-old-installation-media
remote: Permission to ekohl/foreman.git denied to chris1984.
fatal: unable to access 'https://github.com/ekohl/foreman.git/': The requested URL returned error: 403
[vagrant@toledodevel foreman]$ git push ewoud 37606-drop-old-installation-media --force
remote: Permission to ekohl/foreman.git denied to chris1984.

ekohl · 2024-08-14T15:51:19Z

I tried pushing to your branch to add the migration and I am getting this:

In GitHub you can (don't have to) allow maintainers to push to your PRs. I have that enabled, but since you're not a maintainer and I didn't grant you explicit permissions you get a permission denied.

That's why I suggested to submit a PR against my fork or open a fresh PR. If you'd like to finish the whole effort then I think the latter is easier.

chris1984

@ShimShtein I helped get the migration in, it looks fine to me. I checked all the areas and other than test fixtures/tests we have covered all our bases. Let me know if this looks good or if you want me to try and update the tests.

Ewoud wanted to get this in before 3.12.

ekohl · 2024-08-15T15:15:57Z

Rebased and squashed. I also expanded the commit message a bit to better describe what's going on.

app/services/medium_providers/provider.rb

app/controllers/api/v2/media_controller.rb

app/views/media/_form.html.erb

Gauravtalreja1 · 2024-10-22T11:42:41Z

db/seeds.d/100-installation_media.rb

@@ -6,17 +6,7 @@
 Medium.without_auditing do
  [
    {
-      :name => "CentOS 7 mirror",
-      :os_family => "Redhat",
-      :path => "http://mirror.centos.org/centos/$major/os/$arch",


This old CentOS mirror is still available for older CentOS versions, so can we keep both CentOS Stream and CentOS media's, since 8-stream content still exists on old mirrors?

It's not: mirror.centos.org no longer has any DNS records and the whole infrastructure has been retired. And I don't want to set something up pointing to vault.centos.org.

Fair enough, upon checking, I discovered 8-stream has only a readme which states "This directory (and version of CentOS) is deprecated.". So, we're fine with the current approach since 8-stream is already EOL

ekohl · 2024-10-22T12:26:38Z

Rebased and fixed -Stream to -stream.

Gauravtalreja1 · 2024-10-22T17:59:19Z

db/migrate/20240813141845_rename_centos_stream_mirror_to_centos.rb

@@ -0,0 +1,9 @@
+class RenameCentosStreamMirrorToCentos < ActiveRecord::Migration[6.1]
+  def up
+    Medium.unscoped.where(name: "CentOS Stream 9 mirror").update_all(name: "CentOS mirror")


Along with this media, should we consider updating all other CentOS mirrors which we are removing in this migration, such as CentOS 7 mirror and CentOS Stream?

It doesn't remove old media because users may have reused that and pointed it to vault.centos.org.

I suppose users could have introduced one named CentOS mirror which may conflict. I'm not sure what's best here.

How about search by URL and then removing those:

Medium.unscoped.where(url: "http://mirror.centos.org/centos/$major/os/$arch")

I don't like to surprise people and manage the data too much. I think I'll go with error handling where it ignores the rename if CentOS mirror already exists and add an upgrade warning to the release notes.

Confusingly enough, the name is not a unique column so the DB allows multiple entries with the same name. I've come up with a solution that I isn't very pretty, but probably reliable.

I'd appreciate your thoughts on it.

The new migration logic makes sense to me.

The host mirror.centos.org is gone now that CentOS < 9 is EOL. It includes a migration to simplify the name, but doesn't include migrations to remove old entries. That is up to the user because they may have modified them and still use them. Co-authored-by: Chris Roberts <[email protected]>

ekohl · 2024-10-31T11:02:45Z

@Gauravtalreja1 could you take another look?

pr-processor bot added the Not yet reviewed label Jun 27, 2024

chris1984 reviewed Aug 1, 2024

View reviewed changes

ekohl commented Aug 5, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Aug 5, 2024

ekohl mentioned this pull request Aug 5, 2024

Handle comment reviews as a review theforeman/prprocessor#197

Merged

chris1984 reviewed Aug 12, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Aug 14, 2024

chris1984 reviewed Aug 15, 2024

View reviewed changes

ekohl force-pushed the 37606-drop-old-installation-media branch from a0f3206 to 7b76910 Compare August 15, 2024 15:15

ekohl marked this pull request as ready for review August 15, 2024 15:15

pr-processor bot added Needs re-review and removed Waiting on contributor labels Aug 15, 2024

ekohl mentioned this pull request Oct 22, 2024

Add CentOS Stream 10 installation medium #10356

Closed

Gauravtalreja1 reviewed Oct 22, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Oct 22, 2024

ekohl force-pushed the 37606-drop-old-installation-media branch from 7b76910 to 1724bfb Compare October 22, 2024 12:26

chris1984 approved these changes Oct 22, 2024

View reviewed changes

ekohl requested a review from Gauravtalreja1 October 22, 2024 16:48

Gauravtalreja1 reviewed Oct 22, 2024

View reviewed changes

ekohl force-pushed the 37606-drop-old-installation-media branch from 1724bfb to 66654d0 Compare October 28, 2024 17:02

ekohl mentioned this pull request Nov 21, 2024

Fix foreman_provisioning role theforeman/forklift#1866

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37606 - Drop old CentOS installation media #10227

Fixes #37606 - Drop old CentOS installation media #10227

ekohl commented Jun 27, 2024 •

edited

Loading

chris1984 left a comment

chris1984 commented Aug 5, 2024

ekohl left a comment

ekohl Aug 5, 2024

ShimShtein Aug 11, 2024

chris1984 Aug 12, 2024

ekohl Aug 14, 2024

chris1984 left a comment •

edited

Loading

chris1984 left a comment

ekohl commented Aug 14, 2024

chris1984 left a comment

ekohl commented Aug 15, 2024

Gauravtalreja1 Oct 22, 2024 •

edited

Loading

ekohl Oct 22, 2024

Gauravtalreja1 Oct 22, 2024

ekohl commented Oct 22, 2024

Gauravtalreja1 Oct 22, 2024

ekohl Oct 23, 2024

stejskalleos Oct 23, 2024

ekohl Oct 23, 2024

ekohl Oct 28, 2024

stejskalleos Oct 29, 2024

ekohl commented Oct 31, 2024

Fixes #37606 - Drop old CentOS installation media #10227

Are you sure you want to change the base?

Fixes #37606 - Drop old CentOS installation media #10227

Conversation

ekohl commented Jun 27, 2024 • edited Loading

chris1984 left a comment

Choose a reason for hiding this comment

chris1984 commented Aug 5, 2024

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris1984 left a comment • edited Loading

Choose a reason for hiding this comment

chris1984 left a comment

Choose a reason for hiding this comment

ekohl commented Aug 14, 2024

chris1984 left a comment

Choose a reason for hiding this comment

ekohl commented Aug 15, 2024

Gauravtalreja1 Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl commented Oct 31, 2024

ekohl commented Jun 27, 2024 •

edited

Loading

chris1984 left a comment •

edited

Loading

Gauravtalreja1 Oct 22, 2024 •

edited

Loading