Autobuild: Clean up workflow description #2470

hoffie · 2022-03-07T23:00:15Z

This is a small, split-out part of the announced autobuild refactoring.

Short description of changes

Autobuild: Drop commented out code and duplicate newlines
Autobuild: Shorten branch matching syntax
Autobuild: Drop comments which repeat step names
Autobuild: Unify identation
Autobuild: Reorganize, shorten and strip redundant comments
Autobuild: Shorten if expressions
${{ ... }} is optional for if: entries. Therefore drop it in order to reduce clutter.
Autobuild: Line-wrap long if statements
Autobuild: Rename uses_codeql -> run_codeql

CHANGELOG: Autobuild: Refactor for better readability

Context: Fixes an issue?

Improve readability/maintenance.

Does this change need documentation? What needs to be documented and how?

No.

Status of this Pull Request

Ready when CI is green.

What is missing until this pull request can be merged?

Ready when CI is green.

Note: I suggest reading individual commit diffs if unsure. git show -w and Github's whitespace-ignoring diff may also help during review.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

ann0see

Are you really sure about removing this many comments? Of course the "can also be warn,…" can be removed but the links to the documentation or blog posts should remain. Some comments might be trivial (Checkout code,…)

.github/workflows/autobuild.yml

ann0see

I think I'm probably commenting a lot about the structure? But please see below:

.github/workflows/autobuild.yml

ann0see · 2022-03-08T20:45:10Z

.github/workflows/autobuild.yml

          NOTARIZATION_PASSWORD:      ${{ secrets.NOTARIZATION_PASSWORD }}
          KEYCHAIN_PASSWORD:          ${{ secrets.KEYCHAIN_PASSWORD }}

      - name:                       "Post-Build for ${{ matrix.config.config_name }}"


Should probably be named something like

Move files... But that's not part of this PR though.

Yes, that could indeed be improved to match what's actually done in the step (in another PR).

.github/workflows/autobuild.yml

ann0see · 2022-03-08T20:47:04Z

.github/workflows/autobuild.yml

+      - name:                       "Notarize macOS Release Build"
+        if:                         >-
+                                    steps.step_cmd3_postbuild.outputs.artifact_1 != '' &&
+                                    steps.step_macos_build.outputs.macos_signed == 'true' &&
+                                    contains(needs.create_release.outputs.publish_to_release, 'true')
+        id:                         notarize-macOS-app
+        uses:                       devbotsxyz/xcode-notarize@d7219e1c390b47db8bab0f6b4fc1e3b7943e4b3b


Why is this done after the upload artifact call? Couldn't there also be signed non-release artifacts?

Hm, currently it only runs on release builds. It could probably also be run earlier. Not sure if there are any reasons for not signing dev builds, but as this runs on @emlynmac's fork and not on ours anyway, I'm not sure how deeply we should care...? :)

Maybe open a separate issue for that?

@emlynmac could you please comment why this is here and not before any upload has started?

This step is to notarize the build, which has to happen after the build is done and the artifacts created.
The signing of the macOS build is a two-step process; build with certs then notarize the full thing with Apple.

Is this basically a step which transfers something (hashes?) to apple?

There's a bunch of information about how notarization works here: https://developer.apple.com/documentation/security/notarizing_macos_software_before_distribution

@hoffie sorry for the small modification request: Could you please add a comment here that this needs to be done after the build is created?

As far as I understand, in theory it could also be put above (but below the build, since it needs the artifact to be somehow sent to Apple)

Not sure I understand.
Do you mean that the binaries can only be signed once they're built? That would be rather obvious to me (similar for other instances: We don't explicitly highlight that the build dependencies have to be prepared before the build can start).

Or do you mean that the binaries can only be signed once the artifact has been uploaded to the job? I don't think this is the case (or if it is, I don't understand why). The Notarize step references product-path: deploy/${{ steps.step_cmd3_postbuild.outputs.artifact_1 }}. This looks like a filesystem path to me and not like a job artifact path. So I guess the order could be turned around. Adding a comment which states the contrary would seem strange then :)

Maybe you could send a follow-up PR with a suggested wording?

(In general, I think a PR which touches specific code should leave the code in better state than it was before. However, we should be cautious to avoid trying to push to much additional things which could or should be improved into PRs. PRs are getting large and take lots of iterations until they can get merged...)

Yes sure. @emlynmac should open a PR then since he knows what to do. I think we can move it around - at least from my readings.

.github/workflows/autobuild.yml

hoffie · 2022-03-09T11:08:54Z

.github/workflows/autobuild.yml

          NOTARIZATION_PASSWORD:      ${{ secrets.NOTARIZATION_PASSWORD }}
          KEYCHAIN_PASSWORD:          ${{ secrets.KEYCHAIN_PASSWORD }}

      - name:                       "Post-Build for ${{ matrix.config.config_name }}"


Yes, that could indeed be improved to match what's actually done in the step (in another PR).

.github/workflows/autobuild.yml

hoffie · 2022-03-09T12:03:10Z

.github/workflows/autobuild.yml

-     outputs:
+    # Check if we are doing a release or just a normal build.
+    # This must be done before actually building the app to find out where to upload the binaries and if we need to create a Github release.
+    name:                           Create Github release if this is a release build


Looking at the workflow, I think the new name is slightly misleading. This step happens first, before the builds, so it isn't actually creating the Github release, just setting up variables to tell later steps to generate one. So "Prepare" or "Setup" is better, but happy for the rest of the name to be made clearer.

@softins By new name you are referring to the line which this comment is attached to, right?

This job does indeed create the Github release (although without the artifacts, of course). The step some lines below is also named like that.

Example: https://github.com/jamulussoftware/jamulus/runs/5265414702?check_suite_focus=true#step:5:1

Maybe this would be better:

Suggested change

name: Create Github release if this is a release build

name: Create empty Github release if this is a release build

Addition: I just realized that the changelog is generated by anaylze_git_references.py. Generating a file in an "analyze_" script certainly violates the principle of least surprise...

As this is both related to this PR and #2471, I think I'll open a separate Issue to track this and work on it when both PRs are merged. I've adjust the name of the analyse_git_reference.py step for now.

Looking at the workflow, I think the new name is slightly misleading. This step happens first, before the builds, so it isn't actually creating the Github release, just setting up variables to tell later steps to generate one. So "Prepare" or "Setup" is better, but happy for the rest of the name to be made clearer.

@softins By new name you are referring to the line which this comment is attached to, right?

This job does indeed create the Github release (although without the artifacts, of course). The step some lines below is also named like that.

Example: https://github.com/jamulussoftware/jamulus/runs/5265414702?check_suite_focus=true#step:5:1

Ah, ok, I misunderstood. I thought it was just preparing variables for later use. So it's actually creating a release container that will later receive the assets.

My only other comment there would be that the name is getting rather long, and becoming more like a comment or description. Can it be made more concise but still accurate?

In fact, another comment. I've just looked at a run and specifically at the Prepare Auto-Build/Release job. It looks like it does more than creating an empty Github release, but also does required steps for non-releases, but skipping the Remove Release and Create Release steps. So I think the chosen job name needs to reflect that. Or am I completely misunderstanding? (still learning about workflows, I admit!)

My only other comment there would be that the name is getting rather long, and becoming more like a comment or description. Can it be made more concise but still accurate?

Hrm, you are right. I struggle to find good wording though. Maybe we should drop the imperative for job-level names.

I've just adjusted it like that:

Suggested change

name: Create Github release if this is a release build

name: Build vars & Github release (if required)

.github/workflows/autobuild.yml

ann0see · 2022-03-09T21:36:01Z

BTW: See the nightly: https://github.com/jamulussoftware/jamulus/runs/5487103016?check_suite_focus=true

The Create Release step doesn't even output RELEASE_TITLE

.github/workflows/autobuild.yml

.github/workflows/autobuild.yml.orig

ann0see · 2022-03-09T22:28:01Z

I think I‘m happy now. It has probably blown up a bit

hoffie · 2022-03-09T22:33:18Z

Currently still cleaning up the git history. As @softins had some comments as well I'll wait with merging until tomorrow (or explicit feedback).

softins

Looking good. I left a couple of comments regarding step naming, but they are only suggestions, not deal-breakers :)

Approving anyway.

.github/workflows/autobuild.yml

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet

${{ ... }} is optional for if: entries. Therefore drop it in order to reduce clutter. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idif

Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

Although the build step is only referenced later by Mac-specific steps it should not be called "step_mac_build" as that suggests that this step was Mac-only. It isn't, it's the generic step used by all platforms. Therefore, rename accordingly.

ann0see · 2022-03-10T20:53:03Z

Ok. Ready to merge after Android build is finished.

ann0see · 2022-03-11T20:21:14Z

@hoffie please merge (squash merge?) this if it's finished.

hoffie · 2022-03-11T22:09:06Z

squash merge?

Although the PR contains more than a few commits, I'd like to keep it separate to keep the information what was changed for what reasons. I've already squashed all the same-type adjustments (wording improvements are a single commit, for example).

hoffie added this to the Release 3.9.0 milestone Mar 7, 2022

hoffie requested a review from ann0see March 8, 2022 00:04

ann0see reviewed Mar 8, 2022

View reviewed changes

pljones approved these changes Mar 8, 2022

View reviewed changes

ann0see self-requested a review March 8, 2022 18:17

ann0see reviewed Mar 8, 2022

View reviewed changes

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

.github/workflows/autobuild.yml Show resolved Hide resolved

.github/workflows/autobuild.yml Show resolved Hide resolved

hoffie commented Mar 8, 2022

View reviewed changes

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

hoffie force-pushed the autobuild-workflow-cleanup branch from 23cd481 to 1608122 Compare March 8, 2022 20:25

ann0see reviewed Mar 8, 2022

View reviewed changes

hoffie commented Mar 9, 2022

View reviewed changes

hoffie force-pushed the autobuild-workflow-cleanup branch from 1608122 to 5b09e0a Compare March 9, 2022 11:24

hoffie commented Mar 9, 2022

View reviewed changes

hoffie mentioned this pull request Mar 9, 2022

Autobuild: Invert uses_codeql/run_codeql logic #2481

Closed

ann0see reviewed Mar 9, 2022

View reviewed changes

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

.github/workflows/autobuild.yml Show resolved Hide resolved

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

hoffie force-pushed the autobuild-workflow-cleanup branch from 1925fa0 to f62af89 Compare March 9, 2022 21:28

ann0see requested changes Mar 9, 2022

View reviewed changes

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

hoffie force-pushed the autobuild-workflow-cleanup branch from f62af89 to 149e24a Compare March 9, 2022 22:01

ann0see reviewed Mar 9, 2022

View reviewed changes

.github/workflows/autobuild.yml.orig Outdated Show resolved Hide resolved

hoffie force-pushed the autobuild-workflow-cleanup branch from 149e24a to a9421f2 Compare March 9, 2022 22:17

ann0see approved these changes Mar 9, 2022

View reviewed changes

hoffie force-pushed the autobuild-workflow-cleanup branch 2 times, most recently from 68bfdb2 to 0f970a2 Compare March 9, 2022 22:57

softins approved these changes Mar 9, 2022

View reviewed changes

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

.github/workflows/autobuild.yml Outdated Show resolved Hide resolved

hoffie added 3 commits March 10, 2022 21:16

Autobuild: Drop commented out code and duplicate newlines

d700128

Autobuild: Shorten branch matching syntax

4130389

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#filter-pattern-cheat-sheet

Autobuild: Drop comments which repeat step names

f566b2d

hoffie force-pushed the autobuild-workflow-cleanup branch from 0f970a2 to 6d369d4 Compare March 10, 2022 20:19

hoffie and others added 8 commits March 10, 2022 21:20

Autobuild: Unify indentation

9e2fc08

Autobuild: Reorganize, shorten and strip redundant comments

1c8729a

Autobuild: Shorten if expressions

ecc50f8

${{ ... }} is optional for if: entries. Therefore drop it in order to reduce clutter. https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idif

Autobuild: Line-wrap long if statements

e2d093c

Autobuild: Rename uses_codeql -> run_codeql

ae64e38

Autobuild: Make two-space indentation consistent

fe9f415

Autobuild: Clarify action step names and improve comments

080a27d

Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>

hoffie force-pushed the autobuild-workflow-cleanup branch from 6d369d4 to d31bda0 Compare March 10, 2022 20:21

hoffie merged commit 7c1c1e9 into jamulussoftware:master Mar 11, 2022

hoffie deleted the autobuild-workflow-cleanup branch March 19, 2022 20:23

	name: Create Github release if this is a release build
	name: Create empty Github release if this is a release build

	name: Create Github release if this is a release build
	name: Build vars & Github release (if required)

Autobuild: Clean up workflow description #2470

Autobuild: Clean up workflow description #2470

Uh oh!

Conversation

hoffie commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ann0see left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ann0see left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see Mar 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hoffie Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softins Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hoffie Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hoffie commented Mar 7, 2022 •

edited

Loading

ann0see Mar 10, 2022 •

edited

Loading

hoffie Mar 9, 2022 •

edited

Loading

softins Mar 9, 2022 •

edited

Loading

hoffie Mar 9, 2022 •

edited

Loading

ann0see commented Mar 9, 2022 •

edited

Loading