-
Notifications
You must be signed in to change notification settings - Fork 986
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
Fixes #37606 - Drop old CentOS installation media #10227
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShimShtein do you want me to add this to my list of reviews to go over as part of the mentorship?
@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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}, | ||
{ | ||
:name => "CentOS Stream 9 mirror", | ||
:name => "CentOS mirror", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may need a DB migration to rename the existing entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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">]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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 <chrobert@redhat.com>
a0f3206
to
7b76910
Compare
Rebased and squashed. I also expanded the commit message a bit to better describe what's going on. |
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.