Use dot to separate distro major and minor version and replace distro registry with factory #332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thozza
force-pushed
the
dot-notation
branch
3 times, most recently
from
03a97b0
to
0ca994d
Compare
|
I want to integrate this first into the |
|
Commit 6f9ce5a has a |
achilleas-k
reviewed
Dec 21, 2023
|
You can blame @ondrejbudai for the merge conflicts there 😅 |
|
🙈 Sorry about that. :( Should be trivial to rebase, though. |
|
I was looking into why so many images were rebuilt [1] and it looks like the gpg keys aren't in the rpm options any more. Here's a single diff: diff --git a/fedora_39-x86_64-qcow2-empty.json b/fedora_39-x86_64-qcow2-empty.json
index 190b5fe2..8576da1f 100644
--- a/fedora_39-x86_64-qcow2-empty.json
+++ b/fedora_39-x86_64-qcow2-empty.json
@@ -9,9 +9,6 @@
"baseurls": [
"https://rpmrepo.osbuild.org/v2/mirror/public/f39/f39-x86_64-branched-20231101"
],
- "gpgkeys": [
- "-----BEGIN PGP PUBLIC KEY BLOCK-----\n\nmQINBGLykg8BEADURjKtgQpQNoluifXia+U3FuqGCTQ1w7iTqx1UvNhLX6tb9Qjy\nl/vjl1iXxucrd2JBnrT/21BdtaABhu2hPy7bpcGEkG8MDinAMZBzcyzHcS/JiGHZ\nd/YmMWQUgbDlApbxFSGWiXMgT0Js5QdcywHI5oiCmV0lkZ+khZ4PkVWmk6uZgYWf\nJOG5wp5TDPnoYXlA4CLb6hu2691aDm9b99XYqEjhbeIzS9bFQrdrQzRMKyzLr8NW\ns8Pq2tgyzu8txlWdBXJyAMKldTPstqtygLL9UUdo7CIQQzWqeDbAnv+WdOmiI/hR\netbbwNV+thkLJz0WD90C2L3JEeUJX5Qa4oPvfNLDeCKmJFEFUTCEdm0AYoQDjLJQ\n3d3q9M09thXO/jYM0cSnJDclssLNsNWfjJAerLadLwNnYRuralw7f74QSLYdJAJU\nSFShBlctWKnlhQ7ehockqtgXtWckkqPZZjGiMXwHde9b9Yyi+VqtUQWxSWny+9g9\n6tcoa3AdnmpqSTHQxYajD0EGXJ0z0NXfqxkI0lo8UxzypEBy4sARZ4XhTU73Zwk0\nLGhEUHlfyxXgRs6RRvM2UIoo+gou2M9rn/RWkhuHJNSfgrM0BmIBCjhjwGiS33Qh\nysLDWJMdch8lsu1fTmLEFQrOB93oieOJQ0Ysi5gQY8TOT+oZvVi9pSMJuwARAQAB\ntDFGZWRvcmEgKDM5KSA8ZmVkb3JhLTM5LXByaW1hcnlAZmVkb3JhcHJvamVjdC5v\ncmc+iQJOBBMBCAA4FiEE6PI5lvIyGGQMtEy+dc9axBi450wFAmLykg8CGw8FCwkI\nBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdc9axBi450yd4w//ZtghbZX5KFstOdBS\nrcbBfCK9zmRvzeejzGl6lPKfqwx7OOHYxFlRa9MYLl8QG7Aq6yRRWzzEHiSb0wJw\nWXz5tbkAmV/fpS4wnb3FDArD44u317UAnaU+UlhgK1g62lwI2dGpvTSvohMBMeBY\nB5aBd+sLi3UtiSRM2XhxvxaWwr/oFLjKDukgrPQzeV3F/XdxGhSz/GZUVFVprcrB\nh/dIo4k0Za7YVRhlVM0coOIcKbcjxAK9CCZ8+jtdIh3/BN5zJ0RFMgqSsrWYWeft\nBI3KWLbyMfRwEtp7xSi17WXbRfsSoqwIVgP+RCSaAdVuiYs/GCRsT3ydYcDvutuJ\nYZoE53yczemM/1HZZFI04zI7KBsKm9NFH0o4K2nBWuowBm59iFvWHFpX6em54cq4\n45NwY01FkSQUqntfqCWFSowwFHAZM4gblOikq2B5zHoIntCiJlPGuaJiVSw9ZpEc\n+IEQfmXJjKGSkMbU9tmNfLR9skVQJizMTtoUQ12DWC+14anxnnR2hxnhUDAabV6y\nJ5dGeb/ArmxQj3IMrajdNwjuk9GMeMSSS2EMY8ryOuYwRbFhBOLhGAnmM5OOSUxv\nA4ipWraXDW0bK/wXI7yHMkc6WYrdV3SIXEqJBTp7npimv3JC+exWEbTLcgvV70FP\nX55M9nDtzUSayJuEcfFP2c9KQCE=\n=J4qZ\n-----END PGP PUBLIC KEY BLOCK-----"
- ],
"check_gpg": true,
"baseurl": "https://rpmrepo.osbuild.org/v2/mirror/public/f39/f39-x86_64-branched-20231101"
}
@@ -1530,11 +1527,7 @@
]
}
},
- "options": {
- "gpgkeys": [
- "-----BEGIN PGP PUBLIC KEY BLOCK-----\n\nmQINBGLykg8BEADURjKtgQpQNoluifXia+U3FuqGCTQ1w7iTqx1UvNhLX6tb9Qjy\nl/vjl1iXxucrd2JBnrT/21BdtaABhu2hPy7bpcGEkG8MDinAMZBzcyzHcS/JiGHZ\nd/YmMWQUgbDlApbxFSGWiXMgT0Js5QdcywHI5oiCmV0lkZ+khZ4PkVWmk6uZgYWf\nJOG5wp5TDPnoYXlA4CLb6hu2691aDm9b99XYqEjhbeIzS9bFQrdrQzRMKyzLr8NW\ns8Pq2tgyzu8txlWdBXJyAMKldTPstqtygLL9UUdo7CIQQzWqeDbAnv+WdOmiI/hR\netbbwNV+thkLJz0WD90C2L3JEeUJX5Qa4oPvfNLDeCKmJFEFUTCEdm0AYoQDjLJQ\n3d3q9M09thXO/jYM0cSnJDclssLNsNWfjJAerLadLwNnYRuralw7f74QSLYdJAJU\nSFShBlctWKnlhQ7ehockqtgXtWckkqPZZjGiMXwHde9b9Yyi+VqtUQWxSWny+9g9\n6tcoa3AdnmpqSTHQxYajD0EGXJ0z0NXfqxkI0lo8UxzypEBy4sARZ4XhTU73Zwk0\nLGhEUHlfyxXgRs6RRvM2UIoo+gou2M9rn/RWkhuHJNSfgrM0BmIBCjhjwGiS33Qh\nysLDWJMdch8lsu1fTmLEFQrOB93oieOJQ0Ysi5gQY8TOT+oZvVi9pSMJuwARAQAB\ntDFGZWRvcmEgKDM5KSA8ZmVkb3JhLTM5LXByaW1hcnlAZmVkb3JhcHJvamVjdC5v\ncmc+iQJOBBMBCAA4FiEE6PI5lvIyGGQMtEy+dc9axBi450wFAmLykg8CGw8FCwkI\nBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdc9axBi450yd4w//ZtghbZX5KFstOdBS\nrcbBfCK9zmRvzeejzGl6lPKfqwx7OOHYxFlRa9MYLl8QG7Aq6yRRWzzEHiSb0wJw\nWXz5tbkAmV/fpS4wnb3FDArD44u317UAnaU+UlhgK1g62lwI2dGpvTSvohMBMeBY\nB5aBd+sLi3UtiSRM2XhxvxaWwr/oFLjKDukgrPQzeV3F/XdxGhSz/GZUVFVprcrB\nh/dIo4k0Za7YVRhlVM0coOIcKbcjxAK9CCZ8+jtdIh3/BN5zJ0RFMgqSsrWYWeft\nBI3KWLbyMfRwEtp7xSi17WXbRfsSoqwIVgP+RCSaAdVuiYs/GCRsT3ydYcDvutuJ\nYZoE53yczemM/1HZZFI04zI7KBsKm9NFH0o4K2nBWuowBm59iFvWHFpX6em54cq4\n45NwY01FkSQUqntfqCWFSowwFHAZM4gblOikq2B5zHoIntCiJlPGuaJiVSw9ZpEc\n+IEQfmXJjKGSkMbU9tmNfLR9skVQJizMTtoUQ12DWC+14anxnnR2hxnhUDAabV6y\nJ5dGeb/ArmxQj3IMrajdNwjuk9GMeMSSS2EMY8ryOuYwRbFhBOLhGAnmM5OOSUxv\nA4ipWraXDW0bK/wXI7yHMkc6WYrdV3SIXEqJBTp7npimv3JC+exWEbTLcgvV70FP\nX55M9nDtzUSayJuEcfFP2c9KQCE=\n=J4qZ\n-----END PGP PUBLIC KEY BLOCK-----"
- ]
- }
+ "options": {}
},
{
"type": "org.osbuild.selinux",
@@ -5240,11 +5233,7 @@
]
}
},
- "options": {
- "gpgkeys": [
- "-----BEGIN PGP PUBLIC KEY BLOCK-----\n\nmQINBGLykg8BEADURjKtgQpQNoluifXia+U3FuqGCTQ1w7iTqx1UvNhLX6tb9Qjy\nl/vjl1iXxucrd2JBnrT/21BdtaABhu2hPy7bpcGEkG8MDinAMZBzcyzHcS/JiGHZ\nd/YmMWQUgbDlApbxFSGWiXMgT0Js5QdcywHI5oiCmV0lkZ+khZ4PkVWmk6uZgYWf\nJOG5wp5TDPnoYXlA4CLb6hu2691aDm9b99XYqEjhbeIzS9bFQrdrQzRMKyzLr8NW\ns8Pq2tgyzu8txlWdBXJyAMKldTPstqtygLL9UUdo7CIQQzWqeDbAnv+WdOmiI/hR\netbbwNV+thkLJz0WD90C2L3JEeUJX5Qa4oPvfNLDeCKmJFEFUTCEdm0AYoQDjLJQ\n3d3q9M09thXO/jYM0cSnJDclssLNsNWfjJAerLadLwNnYRuralw7f74QSLYdJAJU\nSFShBlctWKnlhQ7ehockqtgXtWckkqPZZjGiMXwHde9b9Yyi+VqtUQWxSWny+9g9\n6tcoa3AdnmpqSTHQxYajD0EGXJ0z0NXfqxkI0lo8UxzypEBy4sARZ4XhTU73Zwk0\nLGhEUHlfyxXgRs6RRvM2UIoo+gou2M9rn/RWkhuHJNSfgrM0BmIBCjhjwGiS33Qh\nysLDWJMdch8lsu1fTmLEFQrOB93oieOJQ0Ysi5gQY8TOT+oZvVi9pSMJuwARAQAB\ntDFGZWRvcmEgKDM5KSA8ZmVkb3JhLTM5LXByaW1hcnlAZmVkb3JhcHJvamVjdC5v\ncmc+iQJOBBMBCAA4FiEE6PI5lvIyGGQMtEy+dc9axBi450wFAmLykg8CGw8FCwkI\nBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdc9axBi450yd4w//ZtghbZX5KFstOdBS\nrcbBfCK9zmRvzeejzGl6lPKfqwx7OOHYxFlRa9MYLl8QG7Aq6yRRWzzEHiSb0wJw\nWXz5tbkAmV/fpS4wnb3FDArD44u317UAnaU+UlhgK1g62lwI2dGpvTSvohMBMeBY\nB5aBd+sLi3UtiSRM2XhxvxaWwr/oFLjKDukgrPQzeV3F/XdxGhSz/GZUVFVprcrB\nh/dIo4k0Za7YVRhlVM0coOIcKbcjxAK9CCZ8+jtdIh3/BN5zJ0RFMgqSsrWYWeft\nBI3KWLbyMfRwEtp7xSi17WXbRfsSoqwIVgP+RCSaAdVuiYs/GCRsT3ydYcDvutuJ\nYZoE53yczemM/1HZZFI04zI7KBsKm9NFH0o4K2nBWuowBm59iFvWHFpX6em54cq4\n45NwY01FkSQUqntfqCWFSowwFHAZM4gblOikq2B5zHoIntCiJlPGuaJiVSw9ZpEc\n+IEQfmXJjKGSkMbU9tmNfLR9skVQJizMTtoUQ12DWC+14anxnnR2hxnhUDAabV6y\nJ5dGeb/ArmxQj3IMrajdNwjuk9GMeMSSS2EMY8ryOuYwRbFhBOLhGAnmM5OOSUxv\nA4ipWraXDW0bK/wXI7yHMkc6WYrdV3SIXEqJBTp7npimv3JC+exWEbTLcgvV70FP\nX55M9nDtzUSayJuEcfFP2c9KQCE=\n=J4qZ\n-----END PGP PUBLIC KEY BLOCK-----"
- ]
- }
+ "options": {}
},
{
"type": "org.osbuild.fix-bls",It's also strange that the builds succeeded when the packages have [1] https://gitlab.com/redhat/services/products/image-builder/ci/images/-/jobs/5804488140 |
|
Also weird: I see the keys in the build log: https://gitlab.com/redhat/services/products/image-builder/ci/images/-/jobs/5804589259 |
|
OH WAIT! Of course everything will get rebuilt. All the distro names have essentially changed so it can't match against old configs (this is okay, we like mass rebuilds from time to time :) ) Now I need to figure out what's happening with my diffs. |
|
Found the issue: I think it'd be nice to support both (of course keep the old one for backwards compatibility). diff --git a/pkg/rpmmd/repository.go b/pkg/rpmmd/repository.go
index 13158b40b..64591a2e5 100644
--- a/pkg/rpmmd/repository.go
+++ b/pkg/rpmmd/repository.go
@@ -18,6 +18,7 @@ type repository struct {
Metalink string `json:"metalink,omitempty"`
MirrorList string `json:"mirrorlist,omitempty"`
GPGKey string `json:"gpgkey,omitempty"`
+ GPGKeys []string `json:"gpgkeys,omitempty"`
CheckGPG bool `json:"check_gpg,omitempty"`
IgnoreSSL bool `json:"ignore_ssl,omitempty"`
RHSM bool `json:"rhsm,omitempty"`
@@ -240,6 +241,9 @@ func LoadRepositoriesFromFile(filename string) (map[string][]RepoConfig, error)
if repo.GPGKey != "" {
keys = []string{repo.GPGKey}
}
+ if len(repo.GPGKeys) > 0 {
+ keys = append(keys, repo.GPGKeys...)
+ }
config := RepoConfig{
Name: repo.Name,
BaseURLs: urls,The reason the build log shows keys is because |
|
I implemented the following changes:
|
achilleas-k
force-pushed
the
dot-notation
branch
from
6cf8aa4
to
58a2e9d
Compare
achilleas-k
previously approved these changes
Jan 2, 2024
Well, I think that it would be for the best to have a PR in osbuild-composer ready, which would integrate the changes there, before we merge this. Because otherwise, we may end up having to make a small change in an image definition, but it would be impossible to make it quickly and re-deploy the service. Unfortunately that means more for for me to keep this PR up-to-date until the PR in composer is ready... 🤷 |
|
Right. Good thinking. Let's see how quickly we can get the osbuild-composer side done then. |
thozza
force-pushed
the
dot-notation
branch
5 times, most recently
from
e4a521c
to
399d74c
Compare
5 tasks
Adjust the test_distro implementation to provide the FactoryFunc in the same way as the regular disto implementation. Consolidate the implementation to ensure that the distro Name always contains a release version. In addition, reduce the exported symbols to limit the unwanted use of them in other packages. Modify affected unit tests, which previously relied on some of the exported symbols from the test_distro. Signed-off-by: Tomáš Hozza <[email protected]>
Add `FromHost()` method, which was previously provided by the distroregistry to get a `distro.Distro` instance representing the host distribution. Signed-off-by: Tomáš Hozza <[email protected]>
Remove the distroregistry package and modify all places which depended on it to use distrofactory. To support listing explicitly supported distros in the reporegistry. Specifically add `NewTestedDefault()` to get reporegistry from testing repositories and `ListDistros()` to list distributions with repositories in a reporegistry. Signed-off-by: Tomáš Hozza <[email protected]>
Simplify the `common.GetHostDistroName()` function to return only distro name (including release version), since the `isBeta` and `isStream` values are not really used by any caller. In addition, keep the dot in the distro name to separate major and minor version. Signed-off-by: Tomáš Hozza <[email protected]>
Some logic related to `reporegistry` will need to change due to introduction of dot to separate major and minor release version in distro names. Both, `images` and `osbuild-composer` have their own copy of the code, which does not scale. In general, the `images` repo needs to handle repositories to generate testing manifests, so it makes sense to keep the authoritative copy of `reporegistry` in this repository and then just use it in `osbuild-composer`. Signed-off-by: Tomáš Hozza <[email protected]>
This will allow to use the function also in `osbuild-composer`, while maintaining only a single copy of the code. I chose this approach instead of making the whole `internal/common` package public. The function is related to distros, so this move also makes some sense. Signed-off-by: Tomáš Hozza <[email protected]>
Factor out the distro-specific ID String parsers into a separate functions in each distro implementation. This allows to introduce the `distroidparser` to have a common and consistent way to parse distro ID Strings, including the use of distro-specific parsers. This will be crucial for backward compatibility of distro names without the dot separator as used by APIs and distro configurations. The parser needs to be a separate package, because adding it to the `distro` package, where the `distro.ID` struct is defined, would result in an import loop. In addition adjust the logic of the RHEL parsing code a bit, to enable returning more sensible errors from the parser function. Cover the newly added code with unit tests. Signed-off-by: Tomáš Hozza <[email protected]>
These should not be used any more. The distro factory should be used instead. Adjust all distro unit tests. Signed-off-by: Tomáš Hozza <[email protected]>
Most importantly, modify the RHEL-7 distro definition to use the minor version in its name. The 'rhel-7' distro name will be defined as an alias to 'rhel-79' in higher layers (osbuild-composer). Adjust all places affected by this change. The distro-specific constructors should not be used any more. The distro factory should be used instead. Adjust all distro unit tests. Signed-off-by: Tomáš Hozza <[email protected]>
The 'rhel-8' distro name will be defined as an alias to the latest GA release in higher layers (osbuild-composer). Adjust all places affected by this change, including deleting the `rhel-8.json` repo definition. The distro-specific constructors should not be used any more. The distro factory should be used instead. Adjust all distro unit tests. Signed-off-by: Tomáš Hozza <[email protected]>
The 'rhel-9' distro name will be defined as an alias to the latest GA release in higher layers (osbuild-composer). Adjust all places affected by this change, including deleting the `rhel-9.json` repo definition. The distro-specific constructors should not be used any more. The distro factory should be used instead. Adjust all distro unit tests. Signed-off-by: Tomáš Hozza <[email protected]>
The `LoadAllRepositories()` will need to be extended with a compatibility layer to eventually support loading distro configurations with and without a dot in their name, under the same distro name. This is to allow us to use dots in new distro config files, while not breaking any existing repo overrides, that the user can have on their system. IOW `rhel-84.json` and `rhel-8.4.json` should both correspond to `rhel-8.4` distro. This will require using the common `distroidstrparser` to parse distro ID string. However, the parser uses distro-specific parsers, which results in an import loop when importing `distroidstrparser` in `rpmmd` package. To break this future import loop, let's move the `Load*Repositories()` with their tests to the `reporegistry` package. This makes some sense, since these are higher-level functions, than just those for loading the actual data from the JSON file. Make necessary adjustments in code which used these functions. The `loadRepositoriesFromFile()` in `rpmmd` is now made public and it is used to load a single repo config from a specific JSON file. Signed-off-by: Tomáš Hozza <[email protected]>
Signed-off-by: Tomáš Hozza <[email protected]>
Rework the reporegistry and loading to of all repo configurations to support distro ID strings with dot to separate minor version. This is done in a backward compatible way, so that e.g. `rhel-89.json` and `rhel-8.9.json` repo configs are considered to be for the same `RHEL-8.9` distro. The backward compatibility is achieved by always using the `distroidstrparser` to first parse any external distro ID string and then transforming the `distro.ID` back to its string representation (which always uses dot to separate minor release version if set). Rework unit tests to verify that repositories with and without a dot in their filename override each other in case they get parsed to the same `distro.ID`. Signed-off-by: Tomáš Hozza <[email protected]>
Let's be consistent and use the dot in the distro name to separate major and minor version. Signed-off-by: Tomáš Hozza <[email protected]>
This is a follow-up to osbuild@81cc5e2 Signed-off-by: Tomáš Hozza <[email protected]>
Rework the gen-manifests to use reporegistry for loading repo configs, instead of own custom implementation. Signed-off-by: Tomáš Hozza <[email protected]>
Rework the cmd/build to use reporegistry for loading repo configs, instead of own custom implementation. Signed-off-by: Tomáš Hozza <[email protected]>
The New() function can be replaced by DistroFactory() in all places. Let's delete it to be consistent with regular distros. Introduce and export `TestDistro1Name` for convenience, because all original callers of `New()` would have to construct the distro ID string on their own. Signed-off-by: Tomáš Hozza <[email protected]>
Some users of the `distrofactory.Factory` have a need to support getting distro instances using distro ID string, which does not correspond to a valid distro. For example, osbuild-composer has the need to allow users to specify RHEL distro names without the minor version (e.g. `rhel-9`), while maintaining the mapping on their side. There are a few limitations: - An alias can't mask explicitly supported distro. - The target of an alias must map to a valid distro. Signed-off-by: Tomáš Hozza <[email protected]>
Let's export ReposByImageTypeName(), which accepts distro, arch and image type names as strings, instead of just `distro.ImageType`. This has the advantage, that the caller may actually use a distro name alias to get repositories, instead of the actual distro name that the image type is associated with. The ReposByImageType() is removed in favor of ReposByImageTypeName(). Signed-off-by: Tomáš Hozza <[email protected]>
Previously, the `LoadAllRepositories()` function used repo file filename as the distro name, without inspecting it. Now that we parse it, the failure to parse the filename as the distro ID string was considered an error. This may cause osbuild-composer fail to start due to failure while loading repositories. Let's use the repo file filename as is as the distro name, if parsing it fails. Signed-off-by: Tomáš Hozza <[email protected]>
|
🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #104
Related to: https://issues.redhat.com/browse/COMPOSER-1962
Main changes:
DistroRegistrywith aDistroFactory. The registry had a fixed list of tested and supported distros, while the factory can generate a distro for any (even untested) version of a supported distro. This will allow building images for new RHEL and Fedora versions, without the need to make any code change.rhel-93orfedora-34). Each distro is now responsible for parsing the distro ID string which corresponds to it. This allows one to define compatibility layers on per-distro basis. a newdistroidparser.Parseris added as the single entry point for parsing any distro ID string with all existing distro-specific parsers..to separate the major and minor release version. This includes theirNameas well as distro ID strings that can be used to construct a distro instance. A backward compatibility for RHEL-8 and RHEL-9 distro ID strings without a dot is part of the implementation.rhel-7,rhel-8orrhel-9. A RHEL distro must always use a minor release version. These minor-version-less distros will be defined as an alias on higher-level (osbuild-composer).Reporegistrynow also supports a dot as the separator for release major and minor version in the repo config filename, when loading repository definitions. It uses thedistroidparser.Parserto parse repo config filename and get a standardized representation of the distro ID string. Meaning thatrhel-88.jsonandrhel-8.8.jsonboth represent repo definitions for RHEL-8.8.