-
Notifications
You must be signed in to change notification settings - Fork 2
*.py files migrated from gardenlinux workflows #179
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
base: main
Are you sure you want to change the base?
*.py files migrated from gardenlinux workflows #179
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
- Coverage 92.95% 92.43% -0.53%
==========================================
Files 28 30 +2
Lines 1306 1797 +491
==========================================
+ Hits 1214 1661 +447
- Misses 92 136 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
description: "Generated GitHub workflow flavors matrix" | ||
version: | ||
description: GardenLinux Python library version | ||
default: "0.10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the version here. Don't see an benefit with parametrizing it here.
pyproject.toml
Outdated
gl-flavors-parse = "gardenlinux.flavors.__main__:main" | ||
gl-oci = "gardenlinux.oci.__main__:main" | ||
gl-s3 = "gardenlinux.s3.__main__:main" | ||
gl-gh = "gardenlinux.github.__main__:main" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last review request is still valid.
@@ -0,0 +1,3 @@ | |||
from .__main__ import create_github_release_notes | |||
|
|||
__all__ = ["create_github_release_notes"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO there's no reason to export this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion calling gardenlinux.github.create_github_release_notes looks much better than gardenlinux.github.main.create_github_release_notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do agree in general you won't call create_github_release_notes()
from any other source than the main entrypoint.
src/gardenlinux/github/__main__.py
Outdated
|
||
s3_artifacts._bucket.download_file( | ||
release_object.key, artifacts_dir.joinpath(f"{cname}.s3_metadata.yaml") | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still would like to see s3_artifacts.bucket
here and everywhere else where used. Reasoning given last time.
src/gardenlinux/github/__main__.py
Outdated
def download_all_metadata_files(version, commitish): | ||
repo = Repo(".") | ||
commit = repo.commit(commitish) | ||
flavors_data = commit.tree["flavors.yaml"].data_stream.read().decode("utf-8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure if flavors.yaml
should be "checked" out here.
As you are touching a local Git repository here I'm not sure if it is useful to reset the checked out commit each time this function is called. You may just access "whatever"
flavors.yaml
is located here instead. Please be aware thatFlavorsParser
will use thefeatures
directory that is located here anyway without ensuring that the commitish matches.
src/gardenlinux/github/__main__.py
Outdated
commitish_short = commitish[:8] | ||
|
||
for flavor in flavors: | ||
cname = CName(flavor[1], flavor[0], "{0}-{1}".format(version, commitish_short)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same request as last time:
You may use the
commit_id
parameter recently added here.
src/gardenlinux/github/__main__.py
Outdated
def release_notes_compare_package_versions_section(gardenlinux_version, package_list): | ||
output = "" | ||
version_components = gardenlinux_version.split(".") | ||
# Assumes we always have version numbers like 1443.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be aware we'll introduce semantic versioning soon. This logic should use a
semver
Python package if suitable and consider the patch version to be zero if only one dot is found in the string.
@vivus-ignis please cherry-pick the changes from gardenlinux/gardenlinux#3485 |
Done. |
Hate to be the party pooper here, but this PR is far too big. There are many different type changes that this PR contains that can be split up easier. I get that this is a refactor, but we can split it into more mindful pieces. Please split this PR into several small PRs. For instance, test data as a standalone PR. Then there is We change the exports from the Why do we add a binary file to this PR? Can we skip this, or can we generate it? |
As agreed with @Akendo, splitting this PR into smaller PRs.
What this PR does / why we need it: This consolidates python CI code from the main gardenlinux repository into the existing python module
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Release creation code was tested on a gardenlinux repo fork: https://github.com/vivus-ignis/gardenlinux-dev/releases
As the bulk of the code was extracted from the gardenlinux/gardenlinux repository, this PR does not show what was changed there. Therefore attaching a diff here:
Diff
Release note: