ci: Finish migration to Github Actions #604

FranzBusch · 2024-07-24T10:24:47Z

Motivation

I made more progress on the reusable workflows in NIO so it's even easier to adopt them.

Modification

This PR fixes up the renamed workflow references and adds unit tests and cxx-interop checks.

Result

Green GH actions CI

# Motivation I made more progress on the reusable workflows in NIO so it's even easier to adopt them. # Modification This PR fixes up the renamed workflow references and adds unit tests and cxx-interop checks. # Result Green GH actions CI

FranzBusch · 2024-07-24T10:34:51Z

@simonjbeaumont @czechboy0 Would appreciate some eyes on the failing format check

czechboy0 · 2024-07-24T10:36:36Z

@simonjbeaumont @czechboy0 Would appreciate some eyes on the failing format check

I suspect you want to add Tests/OpenAPIGeneratorReferenceTests/Resources/ReferenceSources to the ignore list for swift-format.

czechboy0 · 2024-07-24T10:37:19Z

See here:

swift-openapi-generator/scripts/run-swift-format.sh

Line 34 in 9985908

| grep -z -v -e 'Tests/OpenAPIGeneratorReferenceTests/Resources' \

czechboy0 · 2024-07-24T14:08:26Z

@FranzBusch It's still failing, seems the Generated entry isn't working, it's meant to match the word Generated anywhere in the path. The affected paths here are e.g. Examples/manual-generation-generator-cli-example/Sources/ManualGeneratorInvocationClient/Generated/Client.swift

czechboy0 · 2024-07-24T14:16:16Z

Ok, soundness is failing now:

** Running /code/scripts/check-license-headers.sh...
** ERROR: Unsupported file extension for file (exclude or update this script): .swiftformatignore

I think it just requires you to add .swiftformatignore to the ignore list in scripts/check-license-headers.sh

FranzBusch · 2024-07-24T14:27:37Z

@czechboy0 @simonjbeaumont This passes the format check now. Please take a look at the changes. I also added the integration tests and example tests here. Once we have this merged the only thing that is missing is

Turning on the license check and turning off the old CI
Adding the compatibility test

FranzBusch · 2024-07-24T14:29:56Z

Just realised I have to change something in the reusable workflow to get the integration tests to pass

simonjbeaumont · 2024-08-20T08:39:47Z

@FranzBusch noticed this got reopened. Should I assume it's still a WIP for now and wait for a ping from you to review?

FranzBusch · 2024-08-20T09:10:15Z

@simonjbeaumont @czechboy0 This PR is ready to review. The CI is all passing. I only had to skip the unacceptable language mode check and the shell check steps. I leave this up to you to re-enable them. The language mode depends on the old soundness CI to remove the text file with the words and the shell check depends on fixing up a few scripts. I would also encourage you to look into the examples CI and if you can reduce build times by using the same build dirs. You are currently rebuilding the same dependencies multiple times.

FranzBusch · 2024-08-20T09:12:30Z

The compatibility test is also missing but similarly to the above could you please set that up in a follow up. It would be a good exercise to see if the reusable workflows give you want you need.

czechboy0 · 2024-08-20T09:13:01Z

Looks broadly good to me, but I'll let @simonjbeaumont review more closely and sign off.

simonjbeaumont

Thanks for all the work here @FranzBusch! Left some comments.

.github/workflows/pull_request.yml

FranzBusch · 2024-09-09T09:49:56Z

@simonjbeaumont Gentle ping. I think we can merge this. It should replicate everything that Jenkins is currently providing.

simonjbeaumont · 2024-09-09T10:02:03Z

I'll take this, this week. Sorry for letting it linger.

.github/workflows/pull_request.yml

simonjbeaumont

@FranzBusch this is now at a state where I'm happy with it.

I realise this was initially your PR, but could you take a look.

@czechboy0 once @FranzBusch and I are aligned. I think you should give it a look too :)

…ith them enabled

simonjbeaumont · 2024-09-18T16:02:15Z

@FranzBusch I'm happy with this now and so is Honza. Can you do a final pass with your Github Actions hat on and then hit merge? :)

FranzBusch · 2024-09-18T17:20:37Z

.github/workflows/pull_request.yml

Should we track enabling warnings as errors for those pipelines at some point?

More than happy to. But that's over-and-above what we currently had in this project. We can track it separately.

FranzBusch · 2024-09-18T17:20:53Z

.github/workflows/pull_request.yml

+            broken_symlink_check_enabled: true
+            docs_check_enabled: true
+            format_check_enabled: true
+            license_header_check_enabled: true
+            license_header_check_project_name: "SwiftOpenAPIGenerator"
+            shell_check_enabled: true
+            unacceptable_language_check_enabled: true


Why are we enabling them explicitly? The default is true

Because it's more visible. The fact that some are default to true and some to false is confusing IMO. I like to be able to see what tests we're running in the YAML.

I am fine with explicitly stating but just to be clear no check is defaulting to false.

OK, my mistake. Thanks for confirming :)

FranzBusch · 2024-09-18T17:21:16Z

.github/workflows/pull_request.yml

+            matrix_linux_command: "apt-get update -yq && apt-get install -yq jq && ./scripts/run-integration-test.sh"
+            matrix_linux_5_8_enabled: false
+            matrix_linux_nightly_main_enabled: false


Why not run them on the nightly main?

Because it's materially slower and not blocking a PR. Note that this PR does use the nightly Swift versions in the scheduled workflow so we'll still be alerted. I'd really like to only have PR pipelines that are gating PR merges, especially when we have external contributors.

I am surprised. Nightly main is slower than nightly 6.0? If so we should report that since this hints at a compiler speed regression.

No no. It's slower because it's far less likely to have the image available.

What's really annoying (but I realise a Github Action limitation) is that this will still do a bunch of the warmup anyway because the if: false is only at the step-level.

I have some ideas on how we can make that better though, if you're open to them.

I have some ideas here as well. We could dynamically generate the matrix instead of having it hard coded like now.

FranzBusch · 2024-09-18T17:21:47Z

.github/workflows/pull_request.yml

+            matrix_linux_command: "./scripts/test-examples.sh"
+            matrix_linux_5_8_enabled: false
+            matrix_linux_nightly_main_enabled: false


Same here. Why not nightly?

Answered above.

FranzBusch · 2024-09-18T17:22:40Z

docker/docker-compose.yaml

@@ -26,25 +26,25 @@ services:



Do you intend to keep the docker files around?

I hadn't actually considered removing them altogether but I guess the only downside to doing so is that the existing CI will immediately fail because it can't find them. Maybe that's fine since we want it turned off anyway.

@czechboy0 any objections to me just deleting all the docker-based CI files?

No, let's get us moved over fully.

Wait, I've realised that if we do this we'll be full of red X's on all PRs until we get them turned off because they'll still fire on the webhooks. Let's stage this in, get them turned off, then remove the files.

You can turn off the web hooks yourself in the repo settings.

FranzBusch · 2024-09-18T20:38:59Z

I can't approve since I am the author but LGTM!

### Motivation In #604 we enabled all the Github Actions based workflows we need for CI. Since then we have updated the branch protection rules and disabled the webhook for the old CI, so we can remove the Docker Compose files that were used by the old CI. ### Modifications Delete the Docker Compose files. ### Result Less clutter in the repo. ### Test Plan This PR should show the new, Github Actions CI checks, and shouldn't be showing any of the older CI jobs. If that's true, we are safe to remove them.

FranzBusch added 2 commits July 24, 2024 12:26

[GHA] Configure license check

02b8550

Use #!/bin/bash

fa72f86

FranzBusch force-pushed the fb-gha-update branch 2 times, most recently from 1533aed to 62111dd Compare July 24, 2024 10:29

[GHA] Update to the latest workflows

cd74127

# Motivation I made more progress on the reusable workflows in NIO so it's even easier to adopt them. # Modification This PR fixes up the renamed workflow references and adds unit tests and cxx-interop checks. # Result Green GH actions CI

FranzBusch force-pushed the fb-gha-update branch from 62111dd to cd74127 Compare July 24, 2024 10:32

FranzBusch force-pushed the fb-gha-update branch from 3352b91 to 3fbe65e Compare July 24, 2024 14:04

FranzBusch force-pushed the fb-gha-update branch from 3fbe65e to d7a33ba Compare July 24, 2024 14:10

FranzBusch added the semver/none No version bump required. label Jul 24, 2024

FranzBusch force-pushed the fb-gha-update branch from d7a33ba to c42ae5b Compare July 24, 2024 14:14

Add swiftformatignore file

e181199

FranzBusch force-pushed the fb-gha-update branch 2 times, most recently from 22c757e to 2889fa0 Compare July 24, 2024 14:26

FranzBusch closed this Aug 19, 2024

FranzBusch reopened this Aug 19, 2024

FranzBusch force-pushed the fb-gha-update branch 3 times, most recently from a9d8e33 to 793e80b Compare August 19, 2024 18:02

Integration and example tests

828b992

FranzBusch force-pushed the fb-gha-update branch from 793e80b to 828b992 Compare August 20, 2024 09:04

simonjbeaumont reviewed Aug 20, 2024

View reviewed changes

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

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

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

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

FranzBusch mentioned this pull request Sep 9, 2024

[GHA] Call the Soundness workflow #603

Closed

Merge branch 'main' into fb-gha-update

b4d6fd6

simonjbeaumont added 7 commits September 16, 2024 19:50

ci: Update YAML, enable things, cleanup

f69daeb

ci: Add compatibility test pipeline

8c12274

Add docs to CONTRIBUTING.md on running workflows locally

4a96ad5

Remove in-repo soundness scripts

39dd2db

Also remove generate-contributors script

f6721fa

package: Remove DocC package plugin

e434f39

ci: Skip all docker-compose–based CI

a6efc31

simonjbeaumont force-pushed the fb-gha-update branch from 0726458 to a6efc31 Compare September 16, 2024 19:02

simonjbeaumont reviewed Sep 16, 2024

View reviewed changes

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

simonjbeaumont reviewed Sep 16, 2024

View reviewed changes

simonjbeaumont added 4 commits September 18, 2024 14:54

Disable matrix jobs on nightly Swift versions but add scheduled job w…

cc02f93

…ith them enabled

Disable Swift 6 PR pipeline for now

946f9a3

Add newly added malformed-openapi.yaml to license ignore file

4cdf167

Merge remote-tracking branch 'upstream/main' into fb-gha-update

c2ad338

czechboy0 approved these changes Sep 18, 2024

View reviewed changes

simonjbeaumont approved these changes Sep 18, 2024

View reviewed changes

FranzBusch commented Sep 18, 2024

View reviewed changes

simonjbeaumont changed the title ~~[GHA] Update to the latest workflows~~ ci: Finish migration to Github Actions Sep 19, 2024

simonjbeaumont merged commit 632b68f into main Sep 19, 2024
33 checks passed

simonjbeaumont deleted the fb-gha-update branch September 19, 2024 08:26

simonjbeaumont mentioned this pull request Sep 19, 2024

ci: Remove Docker Compose files used by old CI #633

Merged

ci: Finish migration to Github Actions #604

ci: Finish migration to Github Actions #604

Conversation

FranzBusch commented Jul 24, 2024

Motivation

Modification

Result

FranzBusch commented Jul 24, 2024

czechboy0 commented Jul 24, 2024

czechboy0 commented Jul 24, 2024

czechboy0 commented Jul 24, 2024

czechboy0 commented Jul 24, 2024

FranzBusch commented Jul 24, 2024

FranzBusch commented Jul 24, 2024

simonjbeaumont commented Aug 20, 2024

FranzBusch commented Aug 20, 2024

FranzBusch commented Aug 20, 2024

czechboy0 commented Aug 20, 2024

simonjbeaumont left a comment

Choose a reason for hiding this comment

FranzBusch commented Sep 9, 2024

simonjbeaumont commented Sep 9, 2024

simonjbeaumont left a comment

Choose a reason for hiding this comment

simonjbeaumont commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch commented Sep 18, 2024