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

An improved workflow for maintaining Salt #96

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

An improved workflow for maintaining Salt #96

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a8616ca
Initial text for new maintaining workflow for Salt
meaksh Nov 14, 2024
b8bc52b
Add notes and PoC for Salt Extensions
meaksh Nov 27, 2024
69aa9fe
Extra notes about PoC
meaksh Nov 27, 2024
350a21f
Fix wrong reference to file
meaksh Nov 28, 2024
c81cd74
Update accepted/0000-salt-new-maintaining-workflow.md
meaksh Nov 29, 2024
c342ef2
Fix mistake on branch naming
meaksh Nov 29, 2024
1abc6b0
Add minor rephrase and extra notes about OBS structure
meaksh Nov 29, 2024
85f3e08
Add workflow for different changelogs and SCMSYNC integration
meaksh Dec 10, 2024
a8448c9
Adjust changes around SCMSYNC integration
meaksh Dec 10, 2024
ae926f9
Fix note about changelogs for salt-extensions
meaksh Dec 10, 2024
a01c6ab
Add section about tracking upstream and downstream patches
meaksh Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
252 changes: 252 additions & 0 deletions accepted/0000-salt-new-maintaining-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
- Feature Name: An improved workflow for maintaining Salt
- Start Date: 2024-11-14

# Summary
[summary]: #summary

This RFC proposed an improved workflow for maintaining the Salt package for openSUSE/SUSE distributions, and therefore for Uyuni and SUSE Manager.

# Motivation
[motivation]: #motivation

Our current workflow for maintaining Salt requires manual user intervention after the changes are merged into our `openSUSE/salt` codebase, in order to make this changes available in OBS. Moreover, the Salt spec file and patches are tracked in a separated GitHub repository `openSUSE/salt-packaging`, that is also used to generate the changelog entries for the final RPM.

All these steps needs to be performed manually, with the help of some tooling, to eventually create a manual Submit Request to our Salt package in OBS.

With Salt Extensions appearing now in the upcoming Salt 3008 release, we want to introduce a new workflow that suits better and solves some the limitations we currently have.

The purpose of this RFC is:
- Define an new workflow for Salt that reduces user intervention to zero after a given PR is merged in `openSUSE/salt` repository until getting the package ready to consume at OBS.
- Make the Salt maintaining workflow aligned with openSUSE practices.
- Provide a workflow that can also work the same way with packaged Salt Extensions.
- Deprecate the usage of `salt-packaging` repository.

# Detailed design
[design]: #detailed-design

In this new workflow the `openSUSE/salt` GitHub repository will become the unique source of thrust, and it will contain:

- Salt codebase
- Packaging artifacts: spec file, changelog and extra sources
- OBS workflow file

Taking inspiration from the [OBS/SCM integration guide](https://openbuildservice.org/help/manuals/obs-user-guide/cha-obs-scm-ci-workflow-integration), the new workflow will use OBS workflows and GitHub Webhooks to automate pulling the changes from GitHub to OBS.

In addition to this, as new Jenkins job will take care of making the OBS package ready to be submitted to openSUSE or SLE.

This is how the proposed OBS structure would look like:

- `systemsmanagement:saltstack/salt`:
* no services enabled - package ready to be submitted to openSUSE or SLE.
* branched from `systemsmanagement:saltstack:github/salt`
- `systemsmanagement:saltstack:github/salt`:
* services enabled
* package building based on `openSUSE/release/xxxx` GitHub branch.
- `systemsmanagement:saltstack:github:CI:...:PR-XXXX/salt`:
* services enabled
* package building according to PR branch.
* branched and removed automatically from `systemsmanagement:saltstack:github/salt` by OBS workflow.

The same OBS structure will apply to all our OBS targets: `products`, `products:testing` and `products:next`.
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain how the structure applies to these projects concretely? I would have thought products:testing or products:next don't need a separate github subproject.

Copy link
Member Author

@meaksh meaksh Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, actually for products we don't really needed, as we just copypac whatever is in products:testing to products.

But for products:testing and products:next I will also consider the github structure, to be able to have different Salt versions if necessary ensuring those packages are also ready to be consumed (even if those are never be directly released) but we would prevent enabled services can run unexpectely on targets that are linked to products:testing and products:next (like i.a. Uyuni:Master or D:G:M:*)


### Packaging artifacts

All current extra "Sources" files in our RPM package, together with spec file and changelog file will go now to a `pkg/suse/` directory in `openSUSE/salt`:

```
pkg/suse/README.SUSE
Copy link
Member

Choose a reason for hiding this comment

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

Just to make is clear, any patch that we want to apply on top of salt code base should go to this folder pkg/suse/ right?
So basically you are moving all the content from the github project openSUSE/salt-packaging (subfolder salt) to this noew pkg/suse folder (at the start we will not have patches since we start from the same base as upstream)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. We won't be carrying patch files anymore in our new OBS packages, as any code change will automatically inside the source tarball by the obs_scm integration.

The exception to this would be EMBARGOED bugs, where we cannot proceed publicly via GitHub, so we will put a patch file manually in IBS than will be removed the bug is public and we push the changes to GitHub.

Copy link

Choose a reason for hiding this comment

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

A side question; currently, we have a lot of commits that are not in upstream, due to various reasons. With salt-packaging, we can easily check which patches are in upstream, and which patches are not. With removing the patch workflow, we're losing this insight.

Do you perceive this as a problem? Are we still sticking with the "fork & cherrypick commits" development style, or are we moving towards rebasing more frequently?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point! We should probably want to have a way to easily identify what is upstreamed and what is not.

I'll elaborate this a bit on the RFC text. Thanks for the note 👍

Copy link
Member

Choose a reason for hiding this comment

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

thank you @m-czernek , that was exactly my concern when I made the comment. Commits that are not yet upstrem, and currently are maintained on salt-packaging project.
We we start merging commit to our salt project that are not merge upstream yet, it can make it harder to integrate upstream version and know what is merge already or not.
@meaksh thank you for look into this topic.

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've added a section in to RFC to clarify this.

Copy link
Member

Choose a reason for hiding this comment

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

I copied the information from the spec file (PATCH-FIX_UPSTREAM/PATCH-FIX_DOWNSTREAM) to the git log (https://github.com/agraul/salt/commits/metadata-in-git/) to see if that's a better alternative. I guess we're mostly interested in links to upstream (since the downstream PR can already found from the commit anyway), but I added both upstream and downstream URLs for now. For new PRs, we'd want to enforce this kind of metadata in the commit messages, e.g. via a Github action that runs on each PR.

Listing all commits that backport upstream PRs can be done easily with git log --oneline --grep='BACKPORT-UPSTREAM'. Or all commits that are not upstream: git log --oneline --grep='BACKPORT-UPSTREAM' --invert-grep v3006.0-suse..HEAD.Note: this is different from git log --oneline --grep='DOWNSTREAM' because a) the git log includes commits we didn't create patches for and b) some patches have links to both upstream and downstream PRs/commits.
I don't even want to try writing one-liners that do the same for our current salt.spec (give it a go when you have time!)

So far it looks like a good way forward to me, let me know what you think.

P.S.: This current approach rewrote all the commits, we could also put them in git-notes(1) to keep the old commits intact. That's probably a better way, but I had to start somewhere.

pkg/suse/html.tar.bz2
pkg/suse/salt-tmpfiles.d
pkg/suse/transactional_update.conf
pkg/suse/update-documentation.sh
pkg/suse/salt.spec
pkg/suse/salt.changes
```

This is the place now where all those files will be maintained.

#### Salt RPM changelog

As mentioned this is now at `pkg/suse/salt.changes` in `openSUSE/salt` GitHub repo.

When creating a PR to `openSUSE/salt` the user must also include the corresponding changes to the spec file, that can be generated as usual with `osc vc`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I get this point right. Maybe it says spec file but the actual meaning is different, not sure, could you please clarify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, sorry. I meant "changelog" file 😄
Let me fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand it right, that in this case we will add changelog entries manually to the changelog file or manually but with osc vc, still not fully clear here. With the osc services there is way to use commit messages as a changlog items, don't we want to use it this way?

Copy link
Member Author

@meaksh meaksh Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm proposing here to manually add the changelog entry to salt.changes by using the osc vc command directly in your Git tree, so the generated changelog entry (and header) can be included together with the code changes in the PR to openSUSE/salt repo.

This way a single PR to openSUSE/salt repo could contain all possible different bits: code changes, specfile changes, changelog entries, artifacts changes.

See this example PR: https://github.com/meaksh/salt/pull/10/files

Now that you mentioned the "osc services" for the changelog, I realized that the current proposal is not covering the fact that we need to maintain different changelogs depending on the targeted codestreams (as they are not aligned).

I'll have some thoughts and clarify this on the RFC text.

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've added more text to the RFC on how to deal with different maintained changelogs


Similarly to the main Uyuni repository, we should add a GitHub action to warn the user in case no changelog entry is added in the PR.
Copy link
Member

Choose a reason for hiding this comment

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

Are we also merging the changelog like we do in Uyuni or do we need to resolve merge conflicts manually?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be now clarified on the RFC text

Copy link
Member

Choose a reason for hiding this comment

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

I can't find it, can you help me out with the line number or a link? With the list of "backported PRs" in the spec file, we might hit this issue in two places.


### OBS project structure

#### `systemsmanagement:saltstack:github/salt`

This OBS package will only contain `_multibuild` file and a `_service` file that should look like:
Copy link
Member

Choose a reason for hiding this comment

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

Can we have the _multibuild in git as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, indeed probably yes. I'll check and adjust this.

Copy link
Member Author

Choose a reason for hiding this comment

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

After latest changes on the RFC, now _multibuild would be also included in the Git repo.


```
<services>
<service name="obs_scm">
<param name="url">https://github.com/openSUSE/salt.git</param>
<param name="scm">git</param>
<param name="versionformat">@PARENT_TAG@</param>
<param name="versionrewrite-pattern">v(.*)</param>
<param name="revision">openSUSE/release/xxxx</param>
<param name="extract">pkg/suse/salt.*</param>
</service>
<service name="set_version" />
<service name="tar" mode="buildtime"/>
<service name="recompress" mode="buildtime">
<param name="file">*.tar</param>
<param name="compression">gz</param>
</service>
</services>
```

The rest of the files will be automatically pulled by the service, as they are enabled here. This package will be automatically refreshed by OBS at any new commit at `openSUSE/release/xxxx` branch.

#### `systemsmanagement:saltstack/salt`

This OBS package is a branch from `systemsmanagement:saltstack:github/salt`, where we disable the services and manually run them to get the spec file, changelog and obsinfo/obscpio files, so the package can be submitted to openSUSE or SLE.

The `_service` file should look like:

```
<services>
<service name="obs_scm" mode="manual">
<param name="url">https://github.com/openSUSE/salt.git</param>
<param name="scm">git</param>
<param name="versionformat">@PARENT_TAG@</param>
<param name="versionrewrite-pattern">v(.*)</param>
<param name="revision">openSUSE/devel/master</param>
Copy link
Member

Choose a reason for hiding this comment

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

I was not aware we used an openSUSE/devel/master branch. Is this a new branch we will use?

Copy link
Member Author

@meaksh meaksh Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I used openSUSE/devel/master here just as an example, it should point to the eventual openSUSE/release/3008.x branch. I'll fix this on the RFC text.

Currently openSUSE/devel/master is just the devel branch I created with upstream master branch + our patches partially rebased on top (excluding patches to extensions).

<param name="extract">pkg/suse/salt.*</param>
</service>
<service name="set_version" mode="manual" />
<service name="tar" mode="buildtime"/>
<service name="recompress" mode="buildtime">
<param name="file">*.tar</param>
<param name="compression">gz</param>
</service>
</services>
```
And it should only contain the following files:

```
_multibuild
_service
salt-xxxx.obscpio
salt.changes
salt.obsinfo
salt.spec
```

Since services are disabled here, to allow submissions to openSUSE and SLE, this OBS package will be automatically synced with `openSUSE/devel/master` by a Jenkins job.

### OBS and GitHub Webhook integration

As described in the [SCM/CI Workflow integration guide](https://openbuildservice.org/help/manuals/obs-user-guide/cha-obs-scm-ci-workflow-integration#sec-obs-obs-scm-ci-workflow-integration-setup-token-authentication-how-to-authenticate-obs-with-scm), a "GitHub Personal Access Token" must be created and a "GitHub Webhook" configure at `openSUSE/salt` repository.


#### OBS workflow file

A `.obs/workflows.yml` will be also added to `openSUSE/salt` to define the OBS workflow as the following:

```
main_workflow:
steps:
- branch_package:
source_project: systemsmanagement:saltstack:github
source_package: salt
target_project: systemsmanagement:saltstack:github:CI
filters:
event: pull_request

rebuild_master:
steps:
- trigger_services:
project: systemsmanagement:saltstack:github
package: salt
filters:
event: push
branches:
only:
- openSUSE/release/xxxx
```

This workflow will take care of:

- Setting up a new subproject at `systemsmanagement:saltstack:github:CI:...:PR-XXXX/salt` for every incoming PR to build the Salt package according to the changes in the PR.
- Triggering the services at `systemsmanagement:saltstack:github/salt` on any new push to `openSUSE/release/xxxx` to build the package accordingly.

### Making OBS packages ready to be submitted to Maintenance

Since the package at `systemsmanagement:saltstack:github/salt` has "services" enabled, and we cannot enable/disable services using OBS workflows, this means this package is not yet ready to be submitted to openSUSE or SLE, as they don't accept enabled services on their submissions. We must disable the services.

In order to do this we will use an Jenkins job that monitors when a new build is done at `systemsmanagement:saltstack:github/salt` to trigger the following actions at the main `systemsmanagement:saltstack/salt` package:

```
# osc checkout systemsmanagement:saltstack/salt
# cd systemsmanagement:saltstack/salt
# osc service manualrun
# osc commit -m "Push new changes from GitHub"
```

This way, we ensure `salt.spec` and `salt.changes` and obscpio/obsinfo files gets upgraded according to latest changes.

### Proof-of-Concept

I've implemented this structure and automation here:

- GitHub repository: https://github.com/meaksh/salt/ (`openSUSE/devel/master` branch)
- OBS:
* https://build.opensuse.org/package/show/home:PSuarezHernandez:tests/salt
* https://build.opensuse.org/package/show/home:PSuarezHernandez:tests:github/salt
* https://build.opensuse.org/package/show/home:PSuarezHernandez:tests:github:CI:....:PR-XX/salt

- Example PR:
* https://github.com/meaksh/salt/pull/10
* https://build.opensuse.org/package/show/home:PSuarezHernandez:tests:github:CI:meaksh:salt:PR-10/salt

Feel free to open new PRs against `openSUSE/devel/master` to see this in action.

### Salt Extensions

#### Builtin extensions
Copy link
Member

Choose a reason for hiding this comment

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

This has been part of the other RFC, but for me it's still not so clear that we can have "builtin extensions". How do we publish these to PyPI from the main repository? How do we get them to show up on https://extensions.saltproject.io/?

Copy link
Member Author

Choose a reason for hiding this comment

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

These "builtin extensions" I introduced it as an optional way for us to be able to provide extensions that do not have yet a proper Salt Extension package.

This could happen for example in these cases:

  • Not officially published as Salt Extensions yet (sources would come from "salt-extensions-holding" repository) that we want to include but do not want to maintain upstream.
  • Salt Extensions published but not packaged yet in OBS. (in this case we should probably go an package it p)
    might not be necessary but it is an option we have.

Builtin extenions would reduce, if necessary, the number of Salt Extensions packages we want to deal with.

It would be up to us to decide whether we want to have any builtin extensions or we just go with all needed extensions as separeted packages if they are already migrated to Salt Extensions.

Copy link
Member

Choose a reason for hiding this comment

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

In the other RFC, the list of built-in extension includes zypperpkg and transactional_update. These are extensions we want to maintain, i.e. we will need to publish them on PyPI. What's the workflow for maintaining these built-in extensions? Do we have the same rules wrt. pull requests and what they have to contain? How do we publish them to PyPI? What's the plan here?

The sources for the builtin Salt Extensions will be located together with the main Salt codebase at the `openSUSE/salt` GitHub repository. No new packages or subpackages will be created for these extensions as they will be part of the main `python3*-salt` package.

If a fix is needed for any of the builtin extensions, workflow would be the same as for a code fix in the main Salt package.

#### Packaged Salt Extensions

For the Salt Extensions that are packaged separately from the main Salt package, we will create a separated GitHub repository where we will maintain these extensions.

This "openSUSE/salt-extensions" repository will contain:
- a common salt-extension spec file that will generate all RPM packages
- The sources for each Salt Extension we package
- A changelog file
Copy link
Member

Choose a reason for hiding this comment

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

Here we'll need to be careful with merge conflicts. Maybe it won't be a big issue, we probably won't change the extensions that often. On the other hand, we probably update them in batches which could lead to changelog merge conflicts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I just changed this as we would have multiple changelog files, one per maintained workflow.

Of course, merge conflicts could happen for PRs that are introducing different changelog entries at the same, then we would need to rebase the PR before merging it.

- OBS workflow file

When it comes to OBS, we will use the same SCM integration and OBS subprojects schema than the proposed for the main Salt codebase. Unique workflow for Salt and Salt Extensions.

- PoC:
* https://github.com/meaksh/test-repo-1
* https://build.opensuse.org/package/show/home:PSuarezHernandez:tests/salt-extensions

- Example PR:
* https://github.com/meaksh/test-repo-1/pull/6
* https://build.opensuse.org/project/show/home:PSuarezHernandez:tests:github:CI:meaksh:test-repo-1:PR-6

NOTE: We are using a single GitHub repo and single OBS package which provides all different salt-extensions RPM packages. This is preferred against having a separated GitHub repositories and OBS package for each Salt Extension, as it will reduce the number of submissions, maintenance incidents and resources needed.

# Drawbacks
[drawbacks]: #drawbacks

- A bit more complex OBS structure than the current one. Including `obs_scm` service.
- Having to still rely on Jenkins to get the packages ready to be released.

# Alternatives
[alternatives]: #alternatives

1. Stick to our current workflow based on "salt-packaging" -> The workflow doesn't currently fit with Salt Extensions and we don't want to have different workflows between Salt and Salt Extensions.
1. One dedicated GitHub repository and OBS package per each Salt Extension -> It won't save resources and will cause more submissions.
2. The usage of "git submodules" as an alternative to adding the Salt Extensions sources manually would make it tricky to generate patches manually and also to integrate with "obs_scm".
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on those points a bit? In particular, what makes it tricky to generate patches when sources live in separate repositories opposed to separated sub-directories in a single repository? And why is that harder to integrate separate repositories?

I'm not saying we should go that way, I just want to understand the trade-offs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let say we need to prepare a patch in OBS/IBS to fix some code in one extension or even in many of them in a single shot.

Assuming that we use "git submodules" in our Salt Extension repository and we have a sub-directory per extensions which is a "git submodule":

Then if you run git format-patch command on your Git repository root, then you won't get any diff for any of the "git submodules". You have to run git format-patch inside each "git submodule" directory to get a patch file which is actually not relative to your main Git repo but to the submodule repository, so we won't be able to apply that generated patch directly in our spec file but we would need to manually adjust it.

This is why I say that "git submodules" are not really straight forward when it come to generate patches.

When it comes to the obs_scm service (as we want to have a unified workflow for Salt and its extensions), I have not really tested how it behaves with "git submodules".

Hth!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, I understand what you mean now. I agree that using sub-modules in that way is not a good approach. I don't really get the point about patches though, I thought we don't create them anymore with this RFC?

JFI, in the new "SUSE Packaging Git Workflow" git sub-modules are used in "project repositories". Each package that's inside the project is it's own git repository (somewhere else) and they exist inside the project as sub-modules.

Copy link
Member Author

@meaksh meaksh Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the general mainteance, we don't use patches anymore but there are some cases where patches are still needed, like the embargoed bugs, where the fixes are manually pushed to IBS, using a patch file, and not via GitHub repository until the bug is public.


# Unresolved questions
[unresolved]: #unresolved-questions

TBD