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 RPM package generation #2258

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Add RPM package generation #2258

merged 2 commits into from
Feb 8, 2024

Conversation

rtorrero
Copy link
Contributor

@rtorrero rtorrero commented Feb 5, 2024

Description

This PR adds RPM package generation on OBS in addition to the existing container image building. Note that it most likely will fail to build in OBS but after we set up the linking on IBS, there it should build correctly.

How was this tested?

Tested manually by commenting out the irrelevant parts of the CI, and leaving the relevant parts on a fork and on separate project/package in OBS. Linking for IBS was also tested.

@rtorrero rtorrero added ci github_actions Pull requests that update Github_actions code labels Feb 5, 2024
@rtorrero rtorrero marked this pull request as ready for review February 5, 2024 10:48
@rtorrero rtorrero requested review from CDimonaco, arbulu89 and EMaksy and removed request for CDimonaco February 6, 2024 11:26
Copy link
Contributor

@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.

Some suggestions:

  • I think we need to use a normal _service file to create the tarball and set the version to the spec file
  • I would move the container creation files to a container subfolder, as sibling of the rpm folder
  • I wouldn't name the env file env_trento_web. I would name it trento_web.example or similar. We want this being an example. The user must modify it.

packaging/suse/rpm/trento-web.spec Show resolved Hide resolved
packaging/suse/rpm/trento-web.spec Show resolved Hide resolved
export MIX_HOME=/usr/bin
export MIX_REBAR3=/usr/bin/rebar3
export MIX_PATH=/usr/lib/elixir/lib/hex/ebin
echo $LANG
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover echo

install -D -m 0600 packaging/suse/rpm/systemd/env_trento_web %{buildroot}/etc/trento/env_trento_web

%post
%postun
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to add all the service_add_x pre/post entries?

%pre
%service_add_pre trento-web.service

%post
%service_add_post trento-web.service

%preun
%service_del_preun trento-web.service

%postun
%service_del_postun trento-web.service

/etc/trento/env_trento_web

%license LICENSE
%doc CHANGELOG.md README.md
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the CHANGELOG. It will came from the .changes file in %changelog.
In fact, i would add everything under the guide folder if possible

@@ -0,0 +1,14 @@
[Unit]
Description=Trento web service
Copy link
Contributor

Choose a reason for hiding this comment

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

Better Trento Web service

@@ -756,12 +766,12 @@ jobs:
VERSION=$(./hack/get_version_from_git.sh)
# "+" character is not allowed in OBS dockerfile version strings
VERSION=${VERSION//[+]/-}
sed -i 's~%%VERSION%%~'"${VERSION}"'~' packaging/suse/Dockerfile
sed -i 's~%%VERSION%%~'"${VERSION}"'~' $BUILD_STEPS_FILE
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a really bad practice. We should use the _service file to build the tarball and set the format.
It is the most common way of doing it.
Have a look on the agent spec file

runs-on: ubuntu-20.04
if: github.event_name == 'release' || (github.event_name == 'push' && github.ref_name == 'main') || github.event_name == 'workflow_dispatch'
needs: [static-code-analysis, test, test-fe, test-e2e]
strategy:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should use the same strategy.
In the rpm package we should leverage the _service file usage, which makes the process a bit different

@rtorrero
Copy link
Contributor Author

rtorrero commented Feb 7, 2024

Setting it to draft temporarily as I test CI changes

Copy link
Contributor

@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.

Nice!
Thank you for the huge effort

# FIXME: Select a correct license from https://github.com/openSUSE/spec-cleaner#spdx-licenses
License: Apache-2.0
URL: https://github.com/trento-project/web
Source: web.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with the service file?
You should have ${name}-${version}.tar.gz or trento-web-${version}.tar.gz

@rtorrero rtorrero marked this pull request as ready for review February 8, 2024 16:28
@rtorrero rtorrero merged commit aa8226f into main Feb 8, 2024
24 checks passed
@rtorrero rtorrero deleted the rpm-package branch February 8, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci github_actions Pull requests that update Github_actions code
Development

Successfully merging this pull request may close these issues.

None yet

2 participants