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

Add support for CentOS Stream 9 #205

Closed
wants to merge 4 commits into from

Conversation

adarshan-intel
Copy link
Contributor

@adarshan-intel adarshan-intel commented Jun 26, 2024

Description of the changes

Add support for CentOS Stream 9 using the image - quay.io/centos/centos:stream9 to GSC.

How to test this PR?

To test this PR, set follow similar procedure as followed with Ubuntu and do gsc build and signing commands.


This change is Reviewable

Signed-off-by: adarshan-intel <adarsh.anand@intel.com>
Signed-off-by: adarshan-intel <adarsh.anand@intel.com>
Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @adarshan-intel)

a discussion (no related file):
I'd like to request reviews from @jkr0103 and @anjalirai-intel



gsc.py line 264 at r1 (raw file):

    if (os_release['ID'] == 'centos'):
        distro = 'quay.io/centos/centos:stream' + version_id

Aren't you supposed to also check that version_id == 9? Otherwise it seems that your code will try to use quay.io/centos/centos:stream8 even for CentOS 8.


templates/quay.io/centos/centos/apploader.template line 1 at r1 (raw file):

{% extends "apploader.common.template" %}

Isn't this file exactly like templates/centos/apploader.template?

Please instead use the simple:

{% extends "centos/apploader.template" %}

See a similar example for Ubuntu: https://github.com/gramineproject/gsc/blob/master/templates/ubuntu/apploader.template. Recall that Ubuntu is derived from Debian, which is similar to CentOS Stream being derived from CentOS.


templates/quay.io/centos/centos/Dockerfile.build.template line 1 at r1 (raw file):

{% extends "Dockerfile.common.build.template" %}

ditto for all these files -- can't they all be derived from templates/centos/ files?


templates/quay.io/centos/centos/Dockerfile.compile.template line 74 at r1 (raw file):

        python3-scipy \
        python3-voluptuous \
        R-core \

Why do you have these random packages like R?

Signed-off-by: adarshan-intel <adarsh.anand@intel.com>
Signed-off-by: adarshan-intel <adarsh.anand@intel.com>
@adarshan-intel
Copy link
Contributor Author

gsc.py line 264 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Aren't you supposed to also check that version_id == 9? Otherwise it seems that your code will try to use quay.io/centos/centos:stream8 even for CentOS 8.

Done.

@adarshan-intel
Copy link
Contributor Author

templates/quay.io/centos/centos/apploader.template line 1 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Isn't this file exactly like templates/centos/apploader.template?

Please instead use the simple:

{% extends "centos/apploader.template" %}

See a similar example for Ubuntu: https://github.com/gramineproject/gsc/blob/master/templates/ubuntu/apploader.template. Recall that Ubuntu is derived from Debian, which is similar to CentOS Stream being derived from CentOS.

Done.

@adarshan-intel
Copy link
Contributor Author

templates/quay.io/centos/centos/Dockerfile.build.template line 1 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

ditto for all these files -- can't they all be derived from templates/centos/ files?

Done.

@adarshan-intel
Copy link
Contributor Author

templates/quay.io/centos/centos/Dockerfile.compile.template line 74 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Why do you have these random packages like R?

Removed unnecessary packages

@adarshan-intel
Copy link
Contributor Author

Closing this PR as the branch name should follow this convention - name/feature.
Updated PR #206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants