Skip to content

Conversation

@szachovy
Copy link
Member

Description

Solves identified problems in https://github.com/SUSE/spacewalk/issues/28067

ATTENTION: This PR is not properly tested during development due to missing MIs for incoming BVs at the moment. Do not merge. Proper testing will be done with the next BV.

Especially here:

    if mi_id not in node_ids:
        node_ids[mi_id] = url
        custom_repositories[node] = node_ids

I am not fully sure if different URLs with the same MI numbers always contain the same package versions (when yes, when not, where it depends).

@szachovy szachovy self-assigned this Aug 25, 2025
Copy link
Member

@srbarrios srbarrios left a comment

Choose a reason for hiding this comment

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

LGTM. But it will be good to test before merging it.

Copy link
Contributor

@maximenoel8 maximenoel8 left a comment

Choose a reason for hiding this comment

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

Please don't remove amazonlinux2023, we need it for 5.1.
Be careful removing the safe guard for MIs with same value. If it breaks nothing I would prefer to keep it. Those issues are hard to catch.
And I would like more info from releng about the slmicro6X channels on 5.1.

"ubuntu2204_minion": {"/SUSE_Updates_MultiLinuxManagerTools_Ubuntu-22.04_x86_64/"},
"ubuntu2404_minion": {"/SUSE_Updates_MultiLinuxManagerTools_Ubuntu-24.04_x86_64/"}
"ubuntu2404_minion": {"/SUSE_Updates_MultiLinuxManagerTools_Ubuntu-24.04_x86_64/"},
"slmicro60_minion": {"/SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/"},
Copy link
Contributor

@maximenoel8 maximenoel8 Aug 25, 2025

Choose a reason for hiding this comment

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

I don't have the full story of those repositories. By from my experience they were statics. Do they replace

"slmicro60_minion": {
"slmicro6_client_tools": "/SLFO:/Products:/MultiLinuxManagerTools:/SL-Micro-6:/ToTest/product/repo/Multi-Linux-ManagerTools-SL-Micro-6-x86_64/"
},
"slmicro61_minion": {
"slmicro6_client_tools": "/SLFO:/Products:/MultiLinuxManagerTools:/SL-Micro-6:/ToTest/product/repo/Multi-Linux-ManagerTools-SL-Micro-6-x86_64/"
}
?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what static/dynamic here means, but they do not replace this repos because they are empty now (dynamic content). If new tools appear for testing, there will include versions to test (static links). Should I move them here?

Copy link
Contributor

@maximenoel8 maximenoel8 Aug 26, 2025

Choose a reason for hiding this comment

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

We had to separate repository types for the MU .
The dynamic repositories, you only describe the path suffix and the MU number will finish the repository paths. So they will be generated dynamically and be unique for each MU.
The static repositories, the repository path is always the same, releng will push the new packages into this new repository. They will be common for all MUs.
Those two parts are setup differently in the json generator. So adding a static repo to the dynamic section will probably break it.
From my memory, /SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/ don't extend with MU number. Also until now, we were using SLFO:/Products for slmicro6X but those repositories were meant to change. We should be clear which one to use and how they are configured. It's probably a question for releng.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

No they won't have MI numbers for these, the new SLFO workflow says that these repos will always be the same. I thought for this you skip the MI number in the url creation but still create the url without just the number so the result is the original, that's why I approved. If that is not the case, yes we need to keep them static for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is cover with the two static and dynamic dictionaries (see previous explanation)
The issue I was pointing was /SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/ should be in the static dictionary.
But because we already have SLFO path describe for slmicro6.X, I was asking if we need both SLFO and /SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/ or just one.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the JSON generator for susemanager-ci after BV (Build Validation) test results from versions 5.0.5 and 4.3.16. The changes address issues identified in GitHub issue #28067. Note: The PR author has explicitly marked this as untested and not ready for merge.

Changes:

  • Removed openeuler2403_minion from v51 and v43 static configurations and added explicit skip logic for it in dynamic processing
  • Added staging repository paths for slmicro60_minion and slmicro61_minion across both v51 (static and dynamic) and v43 configurations
  • Alphabetically reordered dictionary entries in v51_nodes_static_client_tools_repositories for better maintainability

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
v51_nodes.py Reordered alma9_minion/amazon2023_minion entries alphabetically; removed openeuler2403_minion; added slmicro60/61 to dynamic repos
v43_nodes.py Removed openeuler2403_minion; added ALP/SLFO staging repos to slmicro60_minion and slmicro61_minion
maintenance_json_generator.py Added explicit skip logic for openeuler2403_minion in dynamic repo processing; removed extraneous blank line

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +125 to +127
"slmicro60_minion": {"/SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/"},
"slmicro61_minion": {"/SUSE:/ALP:/Source:/Standard:/1.0:/Staging:/Z/standard/",
"/SUSE:/SLFO:/1.1:/Staging:/Z/standard/"}
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The slmicro60_minion and slmicro61_minion nodes are defined in both the static repositories dictionary (v51_nodes_static_client_tools_repositories at lines 113-118) and the dynamic repositories dictionary (v51_nodes_dynamic_client_tools_repos at lines 125-127). This causes these nodes to have repositories from both sources when get_v51_static_and_client_tools is called. This duplication may lead to unexpected behavior in the maintenance_json_generator where both static and dynamic repositories for the same node are processed. Consider whether these nodes should only be in one dictionary or if the code in maintenance_json_generator.py needs to handle this overlap correctly.

Copilot uses AI. Check for mistakes.

for node, repositories in dynamic_nodes.items():
if node in ["openeuler2403_minion"]:
# openeuler minion are deployed but not used for SLE MU & BV MIs
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The comment states "openeuler minion are deployed but not used for SLE MU & BV MIs" but uses plural "minion" instead of "minions". The grammatically correct form should be "openeuler minions are deployed" or "the openeuler minion is deployed".

Suggested change
# openeuler minion are deployed but not used for SLE MU & BV MIs
# openeuler minions are deployed but not used for SLE MU & BV MIs

Copilot uses AI. Check for mistakes.
custom_repositories = init_custom_repositories(version, static_repos)

for node, repositories in dynamic_nodes.items():
if node in ["openeuler2403_minion"]:
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The condition uses a list with a single element for checking if a node should be skipped. Since there's only one element in the list, this could be simplified to a direct string comparison: if node == "openeuler2403_minion":. However, if more nodes are expected to be added to this skip list in the future, the current list approach is acceptable for extensibility.

Suggested change
if node in ["openeuler2403_minion"]:
if node == "openeuler2403_minion":

Copilot uses AI. Check for mistakes.
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.

5 participants