Skip to content

Add Buildkite#28

Merged
jkmassel merged 9 commits intotrunkfrom
add/buildkite
Feb 6, 2022
Merged

Add Buildkite#28
jkmassel merged 9 commits intotrunkfrom
add/buildkite

Conversation

@jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Feb 4, 2022

Adds Buildkite to the project, and removes CircleCI. Also:

  • Adds Rubocop and fixes related issues.
  • Adds Fastlane to run tests.
  • Replaces use of Carthage with SwiftPM.
  • Disables two tests that didn't work (/cc @leandroalonso)

@jkmassel jkmassel marked this pull request as ready for review February 4, 2022 04:37
@jkmassel jkmassel self-assigned this Feb 4, 2022
@jkmassel jkmassel added the enhancement New feature or request label Feb 4, 2022
@jkmassel jkmassel requested a review from a team February 4, 2022 04:38
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

I checked out this branch locally and verified the SPM setup works (which to be fair was sort of unnecessary, given that CI built fine)

Happy to see you already marked the CI tasks as required 👍

image

My suggestion in #29 is not required for this to be merged into trunk.

aws-eventstream (~> 1, >= 1.0.2)
babosa (1.0.4)
claide (1.0.3)
cocoapods (1.8.4)
Copy link
Contributor

Choose a reason for hiding this comment

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

The latest CocoaPods version at the time of this comment is 1.11.2. Should we take the chance of this PR to upgrade?

The same likely goes for Fastlane, too.

Comment on lines 8 to +10
s.description = <<-DESC
An extensible Media Editor for iOS that allows editing single or multiple images.
DESC
DESC
Copy link
Contributor

Choose a reason for hiding this comment

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

CocoaPods likely does some post processing on this, but the heredoc with the - will keep all the spaces used for indentation here:

image

We could either remove the spaces or use the <<~ heredoc, which strips the left spaces.

In the latter, Rubocop tells me to use two spaces for indentation, so:

Pod::Spec.new do |s|
  # ...
  s.description   = <<~DESC
    An extensible Media Editor for iOS that allows editing single or multiple images.
  DESC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice one! Addressed in ce96d6f

project: 'MediaEditor.xcodeproj',
scheme: 'MediaEditor',
devices: ['iPhone 11'],
deployment_target_version: '14.5',
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried the latest for Xcode 13.0 (used in CI), which I guess would be 15?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I tried this on my machine and it worked

Suggested change
deployment_target_version: '14.5',
deployment_target_version: '15.0',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So...as identified in https://github.com/wordpress-mobile/AztecEditor-iOS/pull/1349/files#diff-dff4b99d4e651ab9d7597876ec8dbe92aa934e42c0fb55f4aadd6c106fc96688R37, this value really doesn't do much (it'll use iOS 15 under the hood no matter what this is set to).

I figure we can run around and update all our projects once:

  1. That PR lands
  2. I'm able to extract the simulator creation bits into the release toolkit, then update all our projects to validate the available simulator versions.

Comment on lines +40 to +47
<SkippedTests>
<Test
Identifier = "MediaEditorHubTests/testDoNotShowActivityIndicatorIfImageIsNotBeingLoaded()">
</Test>
<Test
Identifier = "MediaEditorHubTests/testShowActivityIndicatorWhenSwipingToAnImageBeingLoaded()">
</Test>
</SkippedTests>
Copy link
Contributor

Choose a reason for hiding this comment

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

Whilst skipping these is a temporary tech debt incurrence, I'd like to suggest we use XCTExpectFailure instead.

I opened #29 for this and added a rationale.


TIL that the noun deriving from the verb incur is incurrence, not incursion. However, "tech debt incursion" might still be appropriate ⚔️ ⚔️ ⚔️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tech debt incursion
😱 😂

jkmassel and others added 2 commits February 5, 2022 21:48
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
@jkmassel
Copy link
Contributor Author

jkmassel commented Feb 6, 2022

Merging this to unblock #29, and I'll post a followup soon to address the simulator stuff Gio mentioned.

@jkmassel jkmassel merged commit 8e3d163 into trunk Feb 6, 2022
@jkmassel jkmassel deleted the add/buildkite branch February 6, 2022 05:11
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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants