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

Make "shaptools" available inside the Salt Bundle (venv-salt-minion) (bsc#1212695) #76

Merged

Conversation

meaksh
Copy link
Member

@meaksh meaksh commented Oct 5, 2023

This is a proposal for a solution of the problem raised here: https://bugzilla.suse.com/show_bug.cgi?id=1212695

This PR makes use of RPM Supplements attribute to trigger the installation of the python3-shaptools-venv-salt-minion package when both venv-salt-minion and python3-shaptools packages are going to be installed on the system.

This new python3-shaptools-venv-salt-minion package just takes care of creating the necessary symlink to make shaptools available inside the Salt Bundle.

If you want to give it a try:

Tracks: https://github.com/SUSE/spacewalk/issues/22411

This commit makes use of RPM Supplements attribute to trigger the
installation of the "python3-shaptools-venv-salt-minion" package when
"venv-salt-minion" and "python3-shaptools" packages are going to be
both installed on the system.

This new "python3-shaptools-venv-salt-minion" just takes care of
creating the necessary symlink to make "shaptools" available inside the
Salt Bundle.
@yeoldegrove yeoldegrove requested a review from arbulu89 October 16, 2023 12:18
@yeoldegrove
Copy link
Collaborator

@arbulu89 What do you think about this approach?

Copy link
Collaborator

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

@yeoldegrove Looking from a far distance, it looks reasonable.
I have not tested myself, so I guess the user would need install the python3-shaptools-venv-salt-minion package to make the formulas work

@@ -52,6 +52,17 @@ BuildArch: noarch
%description
API to expose SAP HANA functionalities

%package -n python3-shaptools-venv-salt-minion
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see python3 used always. What if we need the py2 version?
Or is it irrelevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the "venv-salt-minion" package is only Python 3.

Are you supporting shap based formulas on Python 2 OSes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We were when I implemented this package 5 years ago. Right now, I'm not really sure to be honest

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also simply not support the py2 version use case in the "venv-salt-minion" use case.
Maybe a simple %if %{without python2} condition could be wrapped around the sub-package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the notes 👍

So Salt Bundle is only Python 3, and from what I can see, formulas like "saphanabootstrap" are installing python3-shaptools depending on the pythonversion that is processing the Salt formula: https://github.com/SUSE/saphanabootstrap-formula/blob/main/hana/packages.sls#L32

Therefore if Salt Bundle is the one processing the formula, which is always Python 3, then the "python3-shaptools" package will be installed and, due to this PR, the new python3-shaptools-venv-salt-minion package will get automatically installed. We are all fine here.

It doesn't really make to have a python 2 flavor of this integration with the Salt Bundle.

I would also simply not support the py2 version use case in the "venv-salt-minion" use case. Maybe a simple %if %{without python2} condition could be wrapped around the sub-package?

Agree as mentioned. It doesn't make sense a Python 2 flavor for the "venv-salt-minion".

IIUC the %if %{without python2} is not really needed here - we always want to build the python3-shaptools-venv-salt-minion package, AFAICS the "python-shaptools" always build python3 subpackages and for some OSes it additionally builds the python2 flavor.

@meaksh
Copy link
Member Author

meaksh commented Oct 17, 2023

@yeoldegrove Looking from a far distance, it looks reasonable. I have not tested myself, so I guess the user would need install the python3-shaptools-venv-salt-minion package to make the formulas work

Hey @arbulu89 , thanks for having a look here :)

This new python3-shaptools-venv-salt-minion package doesn't need to be explicitely installed, it is automatically installed if both python3-shaptools and venv-salt-minion package are going to be installed at the same time (regardless whether one package is installed before or after the other package). This is the "magic" that "RPM Supplements" does.

BTW this mechanism won't work in case "recommends" packages are disabled (like inside containers).

Hth!.

@yeoldegrove
Copy link
Collaborator

@meaksh This would be ready to merge from my perspective, if you did not find anything that speaks against it in the meantime.
After your go, we should try to get this into the product.

Copy link
Collaborator

@yeoldegrove yeoldegrove left a comment

Choose a reason for hiding this comment

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

👍

@yeoldegrove
Copy link
Collaborator

@meaksh
Copy link
Member Author

meaksh commented Nov 21, 2023

@meaksh Please bump the version in https://github.com/SUSE/shaptools/blob/main/shaptools/__init__.py, https://github.com/SUSE/shaptools/blob/main/_service and https://github.com/SUSE/shaptools/blob/main/python-shaptools.changes

@yeoldegrove thanks for the review and sorry for the delay here!

I just pushed a new commit with the version bump as you requested. This should be ready now 👍

@yeoldegrove yeoldegrove merged commit c560630 into SUSE:main Nov 27, 2023
@meaksh meaksh deleted the main-add-integration-with-venv-salt-minion branch November 27, 2023 09:02
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.

3 participants