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.

Sign up for GitHub

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

Jump to bottom

feat: add suffix to the s390x templates #649

Merged
merged 1 commit into from
Dec 9, 2024
Merged

feat: add suffix to the s390x templates #649

merged 1 commit into from
Dec 9, 2024

Conversation

nestoracunablanco
Copy link
Contributor

@nestoracunablanco nestoracunablanco commented Nov 27, 2024 •
edited
Loading

This change also involves renaming the templates themselves. The
end-to-end (e2e) tests require minimal adjustments.

The following templates will be generated for s390x:

  • CentOS Stream 9
  • Fedora
  • RHEL 8
  • RHEL 9
  • Ubuntu

What this PR does / why we need it: s390x enablement

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 27, 2024
0xFelix
0xFelix reviewed Nov 27, 2024
View reviewed changes
generate-templates.yaml Outdated
@@ -62,7 +62,7 @@
- name: Generate RHEL 8 templates
template:
src: rhel8.tpl.yaml
dest: "{{ playbook_dir }}/dist/templates/{{ os }}-{{ item.workload }}-{{ item.flavor }}.yaml"
dest: "{{ playbook_dir }}/dist/templates/{{ os }}-{{ item.workload }}-{{ item.flavor }}{{ '' if target_arch == 'x86_64' else '-' + target_arch }}.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to separate the generated templates also by path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do it if you’d like, but this would require changes to the following commits as well. Ultimately, the release will consist of three files: one containing all architectures and one for each supported architecture.

What are your thoughts, @akrejcir, @jcanocan, @ksimon1, @lyarwood?

Copy link
Member

Choose a reason for hiding this comment

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

Ultimately, the release will consist of three files: one containing all architectures and one for each supported architecture.

As long as it integrates nicely with SSP we can keep it that way.

@nestoracunablanco
Copy link
Contributor Author

/retest

1 similar comment
@nestoracunablanco
Copy link
Contributor Author

/retest

@jcanocan
Copy link

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2024
@nestoracunablanco
Copy link
Contributor Author

/retest

1 similar comment
@nestoracunablanco
Copy link
Contributor Author

/retest

@ksimon1
Copy link
Member

ksimon1 commented Nov 28, 2024

Would you please specify in commit message which templates have the s390x version?
Other s390x templates (like windows) will be introduced in next PRs?
/cc @fossedihelm

@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from 8a7b2f3 to 756b0fa Compare November 28, 2024 13:24
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2024
@fossedihelm
Copy link
Contributor

/hold
I think rhel9.tpl.yaml is missing a change at line 54
Also why don't we just pass the suffix as var from generate-templates.yaml:

    vars:
      [other vars...]
      suffix: "{{ '' if target_arch == 'x86_64' else '-' + target_arch }}"

And then just do"

  name: {{ os }}-{{ item.workload }}-{{ item.flavor }}{{ suffix }}

?

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2024
@nestoracunablanco
Copy link
Contributor Author

nestoracunablanco commented Nov 28, 2024 •
edited
Loading

@ksimon1 comment updated. No new s390x templates are planned for now.
@fossedihelm nice catch. Will do.

@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from 756b0fa to efd3183 Compare November 28, 2024 13:49
fossedihelm
fossedihelm reviewed Nov 28, 2024
View reviewed changes
generate-templates.yaml Outdated
@@ -27,7 +29,7 @@
- name: Generate RHEL 9 templates
template:
src: rhel9.tpl.yaml
dest: "{{ playbook_dir }}/dist/templates/{{ os }}-{{ item.workload }}-{{ item.flavor }}.yaml"
dest: "{{ playbook_dir }}/dist/templates/{{ os }}-{{ item.workload }}-{{ item.flavor }}{{ '' if target_arch == 'x86_64' else '-' + target_arch }}.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the suffix here too :)

@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from efd3183 to e03adcc Compare November 28, 2024 14:41
fossedihelm
fossedihelm approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@fossedihelm fossedihelm left a comment

Choose a reason for hiding this comment

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

Thank you!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2024
@nestoracunablanco
Copy link
Contributor Author

/retest

1 similar comment
@nestoracunablanco
Copy link
Contributor Author

/retest

@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from e03adcc to 0d85b86 Compare November 29, 2024 09:42
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2024
@akrejcir
Copy link
Contributor

Can you consider adding a new label on the template that would contain the architecture? Then we can easily filter templates based on it.

@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from 0d85b86 to 573797e Compare November 29, 2024 13:06
@nestoracunablanco nestoracunablanco mentioned this pull request Nov 29, 2024
Merged
@nestoracunablanco
Copy link
Contributor Author

/retest

2 similar comments
@nestoracunablanco
Copy link
Contributor Author

/retest

@nestoracunablanco
Copy link
Contributor Author

/retest

0xFelix
0xFelix reviewed Dec 2, 2024
View reviewed changes
templates/centos-stream9.tpl.yaml Outdated
@@ -49,13 +49,14 @@ metadata:
{% if item.default %}
template.kubevirt.io/default-os-variant: "true"
{% endif %}
architecture: "{{ 'amd64' if target_arch == 'x86_64' else target_arch }}"
Copy link
Member

Choose a reason for hiding this comment

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

The label should be prefixed with template.kubevirt.io/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nestoracunablanco
feat: add suffix to the s390x templates
6bc94e4 
This change also involves renaming the templates themselves. The
end-to-end (e2e) tests require minimal adjustments.

The following templates will be generated for s390x:
- CentOS Stream 9
- Fedora
- RHEL 8
- RHEL 9
- Ubuntu

Signed-off-by: Nestor Acuna Blanco <[email protected]>
@nestoracunablanco nestoracunablanco force-pushed the feat/suffixForS390xTemplates branch from 573797e to 6bc94e4 Compare December 2, 2024 10:05
@fossedihelm
Copy link
Contributor

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2024
0xFelix
0xFelix reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2024
@fossedihelm
Copy link
Contributor

/retest

fossedihelm
fossedihelm approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@fossedihelm fossedihelm left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
@akrejcir want to take a final look?

@kubevirt-bot kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Dec 9, 2024
@akrejcir
Copy link
Contributor

akrejcir commented Dec 9, 2024

/lgtm

@fossedihelm
Copy link
Contributor

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2024
@kubevirt-bot kubevirt-bot merged commit 019e05a into kubevirt:master Dec 9, 2024
19 checks passed
@nestoracunablanco nestoracunablanco deleted the feat/suffixForS390xTemplates branch December 9, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix left review comments

@fossedihelm fossedihelm fossedihelm approved these changes

Assignees

@akrejcir akrejcir

@fossedihelm fossedihelm

@jcanocan jcanocan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nestoracunablanco @jcanocan @ksimon1 @fossedihelm @akrejcir @kubevirt-bot @0xFelix