An improved workflow for maintaining Salt #96
Conversation
meaksh
force-pushed
the
master-new-maintaining-workflow-for-salt
branch
from
00367fd to
a8616ca
Compare
|
|
||
| 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`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Woops, sorry. I meant "changelog" file 😄
Let me fix this
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've added more text to the RFC on how to deal with different maintained changelogs
|
|
||
| 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 changes file, that can be generated as usual with `osc vc`. |
There was a problem hiding this comment.
Does this work for the different changelogs we currently maintain in parallel?
There was a problem hiding this comment.
I missed to cover on the current proposal our requirement on maintaining different changelogs for the different target codestream we maintain.
I'll add some more text to cover these cases.
There was a problem hiding this comment.
I've added more text to the RFC on how to deal with different maintained changelogs
|
|
||
| When creating a PR to `openSUSE/salt` the user must also include the corresponding changes to the changes file, that can be generated as usual with `osc vc`. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
Are we also merging the changelog like we do in Uyuni or do we need to resolve merge conflicts manually?
There was a problem hiding this comment.
This should be now clarified on the RFC text
|
|
||
| #### `systemsmanagement:saltstack:github/salt` | ||
|
|
||
| This OBS package will only contain `_multibuild` file and a `_service` file that should look like: |
There was a problem hiding this comment.
Can we have the _multibuild in git as well?
There was a problem hiding this comment.
Hmmm, indeed probably yes. I'll check and adjust this.
There was a problem hiding this comment.
After latest changes on the RFC, now _multibuild would be also included in the Git repo.
| * 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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:*)
|
|
||
| ### 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. |
There was a problem hiding this comment.
To me it sounds like a conceptual problem using services to sync the sources. An alternative we could at least look at is using src.opensuse.org as the git forge (according to https://openbuildservice.org/help/manuals/obs-user-guide/cha-obs-scm-bridge#sec-obs-obs-scm-bridge-update-notifications this does not require a _service to sync the sources)
There was a problem hiding this comment.
Hmmm, interesting. I'll give it a try 👍
There was a problem hiding this comment.
I've added some notes about possible usage of SCMSYNC feature from OBS, but after my tests, it does not really work fine when it comes to integrate with the SCM/CI workflow, so the changes on a PR are not actually reflected in the :github:CI:PR-XXXX package as seems to be interfering with the SCMSYNC flag.
I gave it a try here:
- OBS package: https://build.opensuse.org/package/show/home:PSuarezHernandez:tests:github/fake-package
- Test PR: TEST PR 3: Revert "Fake commit" meaksh/salt#12
- OBS PR package: https://build.opensuse.org/package/show/home:PSuarezHernandez:tests:github:CI:meaksh:salt:PR-12/fake-package
^^ notice how salt.changes on the OBS PR package is not reflecting the actual changes on the PR, and also missing branch_request file as we would expect.
I opted for not using SCMSYNC and just carrying the _service file out of the GitHub repo, which might be actually convenient as differnt OBS/IBS maintained project would has some particularities in its _service file, i.a. to point to the right changelog file, so maybe it is easier to just keep them in OBS instead of bringing all those particular files also to Git repo.
| <param name="scm">git</param> | ||
| <param name="versionformat">@PARENT_TAG@</param> | ||
| <param name="versionrewrite-pattern">v(.*)</param> | ||
| <param name="revision">openSUSE/devel/master</param> |
There was a problem hiding this comment.
I was not aware we used an openSUSE/devel/master branch. Is this a new branch we will use?
There was a problem hiding this comment.
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).
|
|
||
| ### Salt Extensions | ||
|
|
||
| #### Builtin extensions |
There was a problem hiding this comment.
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/?
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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.
|
|
||
| 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". |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Read the RFC here