-
Notifications
You must be signed in to change notification settings - Fork 10
Add GitHub Actions workflow for automated PPA releases #103
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 GitHub Actions workflow for automated PPA releases #103
Conversation
rtibbles
left a comment
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 think we probably need to have the full version name.
I also have some questions about some parts of the launchpad lib usage.
.github/workflows/build_debian.yml
Outdated
| - name: Extract version from changelog | ||
| id: changelog_version | ||
| run: | | ||
| CHANGELOG_VERSION=$(dpkg-parsechangelog -S Version | sed -rne 's,([^-\+]+)+(\+dfsg)*.*,\1,p'i) |
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 wonder if we should require that the version information include the fully specified version? So something like 0.5.0-0ubuntu2 - otherwise we wouldn't be able to trigger a 'packaging only' release using this workflow.
| - name: Extract version from release tag | ||
| id: version | ||
| run: | ||
| VERSION=${GITHUB_REF#refs/tags/v} |
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.
This syntax for removing the leading text from a variable was new to me, neat!
.github/workflows/build_debian.yml
Outdated
| - name: Import GPG key | ||
| run: | | ||
| echo -n "${{ secrets.GPG_SIGNING_KEY }}" | base64 --decode | gpg --import --no-tty --batch --yes | ||
| - name: Build unsigned package |
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.
Do these need to be separate steps?
.github/workflows/build_debian.yml
Outdated
| env: | ||
| GPG_SIGNING_KEY: ${{ secrets.GPG_SIGNING_KEY }} | ||
| run: | | ||
| echo "Building unsigned package..." |
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.
Maybe let's move any relevant echo comments into the Makefile?
Makefile
Outdated
| dpkg-buildpackage -S -us -uc --output-directory=dist/ | ||
|
|
||
| # build and sign (signing uses environment GPG_PASSPHRASE and KEYID) | ||
| sign-and-dist: |
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.
We could add dist as a required build step for this, and that would allow us just to call sign-and-dist in the workflow? Feels like the name implies it would do both!
|
|
||
| def get_launchpad_client(): | ||
| cache_dir = os.environ.get("LP_CACHE_DIR", "/tmp/launchpadlib-cache") | ||
| creds_file = os.environ.get("LP_CREDENTIALS_FILE") # set by CI |
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.
It seems that the launchpadlib itself already uses this env var, without us having to intercept and pass through? https://git.launchpad.net/launchpadlib/tree/src/launchpadlib/launchpad.py#n535
|
|
||
| def get_supported_series_dynamically(source_series): | ||
| try: | ||
| out = subprocess.check_output(["ubuntu-distro-info", "--supported", "--series"], text=True).strip() |
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.
Running this command: ubuntu-distro-info --supported --series
Gave me this output:
ubuntu-distro-info: option --series' requires an argument SERIES`
I think just ubuntu-distro-info --supported will do for now? We may want to expand it later, but I'm not exactly sure how we want to do that!
| def install_request_counter(): | ||
| import httplib2 | ||
| orig = httplib2.Http.request | ||
|
|
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 assume this is just your IDE adding this formatting - maybe we should add pre-commit at some point?
|
|
||
| class LaunchpadWrapper(object): | ||
| application_name = 'ppa-gtimelog-copy-packages' | ||
| application_name = 'ppa-kolibri-server-copy-packages' |
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 guess we know where we stole this from originally now!
scripts/create_lp_creds.py
Outdated
| CACHE_DIR = "/absolute/path/to/lp-cache" | ||
| CREDS_FILE = "/absolute/path/to/launchpad.credentials" | ||
|
|
||
| Launchpad.login_with(APP_NAME, "production", cache_dir=CACHE_DIR, credentials_file=CREDS_FILE) |
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.
Should we just output this to the current working directory rather than an absolute path? Also see above for my comment about the cache dir.
rtibbles
left a comment
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.
We've done all we can in review - next test will be doing it live!
Summary
Setup Steps
Create GitHub Environment:
Generate Launchpad Credentials:
Add GitHub Secrets:
Add repository secrets:
References
closes #102
Reviewer guidance