Skip to content
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

New works-for-everything CI #2365

Merged
merged 2 commits into from May 26, 2020
Merged

New works-for-everything CI #2365

merged 2 commits into from May 26, 2020

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented May 24, 2020

This version of the test workflow is designed to be pasted unchanged into any repo whose tests don't have additional dependencies (such as fluent-kit) and don't need to separately test compatibility with Vapor (such as console-kit).

Carries these improvements over the existing CI that was in use for this repo:

  • Runs on pushes to master as well as PRs (in case of changes not made by PR, or cases where the PR's checks weren't allowed to finish)
  • Full coverage of all available Swift runner images for Linux (the new 5.2 ones aren't available yet so aren't listed)
  • Uses latest Xcode without hardcoding on macOS
  • Pretty names for each test step ("Checking out code" instead of "actions/checkout@v2" etc.)
  • Pretty name for the workflow itself ("Test Matrix" instead of "test")

Affects CI only, no semver label attached.

Hopefully there's a way to do two important things that would make this much nicer (above and beyond the improvements already given):

  1. If we could get GitHub to read an entire workflow from an external repository ("actions" don't suffice for this), it would be very nice to be able to have just the needed three versions of this workflow in a vapor-ci repo that would apply to every other repo at once so we wouldn't have to keep updating them one at a time as more innovations are added.

  2. In the same vein, it would be nice if the list of Swift runner images wasn't static, so that the addition of new runner images also didn't necessitate rolling out updates across all the repos.

It's not clear that GitHub's workflows are capable of either of these levels of abstraction - in fact there are strong suggestions that it not only isn't, but doesn't even want to be. That's never stopped me before, of course, but there's also a limit to the time that should be spent on such things. If anyone has suggestions for how to make these happen, please share!

This version of the test workflow can be pasted without changes into any repo whose tests don't have additional dependencies and don't need to separately test that they're still Vapor-compatible (such as console-kit). Improvements over existing CI:

- Runs on pushes to master as well as PRs
- Full coverage of all available Swift runner images for Linux
- Uses latest Xcode without hardcoding on macOS
- Nice names for each test step
- Nice name for the workflow itself
@gwynne gwynne added the enhancement New feature or request label May 24, 2020
@gwynne gwynne requested review from tanner0101 and MrLotU May 24, 2020 11:52
@gwynne gwynne self-assigned this May 24, 2020
Copy link
Sponsor Member

@MrLotU MrLotU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Poke @tanner0101

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
@gwynne gwynne merged commit 1a04e4e into master May 26, 2020
@gwynne gwynne added this to Done in Vapor 4 via automation May 26, 2020
@gwynne gwynne deleted the clean-generic-ci-workflow branch May 26, 2020 17:19
nachoBonafonte pushed a commit to scope-demo/vapor that referenced this pull request May 27, 2020
* New works-for-everything CI (vapor#2365)

* New works-for-everything CI

This version of the test workflow can be pasted without changes into any repo whose tests don't have additional dependencies and don't need to separately test that they're still Vapor-compatible (such as console-kit). Improvements over existing CI:

- Runs on pushes to master as well as PRs
- Full coverage of all available Swift runner images for Linux
- Uses latest Xcode without hardcoding on macOS
- Nice names for each test step
- Nice name for the workflow itself

* Apply suggestions from code review

* `Environment` cleanups (vapor#2353)

* Remove numerous unnecessary code stanzas.

* Don't check `isRelease` in `Environment.==`, it will always be equal because the value is effectively a compile-time constant. Also improve the use of MARK comments so Xcode renders the intended divisions nicely.

* Add warning to `Environment.isRelease` explaining the seemingly unusual behavior of its value.

* Redo the documentation comments for `Environment.secret()` (both versions). Significantly simplify the implementation of the path-taking version. IMPORTANT NOTE: This is an interim step of cleanup while a much more complete revamping of this API is worked on.

* Correctly sanitize the excess arguments Xcode passes to test invocations for the testing environment. I forgot to mention in a previous commit that support was added for `VAPOR_ENV` too...

* Add sanitization of raw SwiftPM's invocation of the xctest runner binary. This is necessarily a little specific to the version of SwiftPM and Xcode involved, but should at least be specific enough a check to not interfere with normal operations if the call sequence changes.

* There is no need to hardcode all the logger levels for `LosslessStringConvertible` conformance. `Logger.Level` is already `RawRepresentable` as `String` and that conformance can be used transparently. (And it has `CaseIterable` for good measure, which provides yet another way to scale this particular elevation.)

Co-authored-by: Gwynne Raskind <gwynne@darkrainfall.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Vapor 4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants