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

Implement sync testflight config command #203

Draft
wants to merge 30 commits into
base: master
Choose a base branch
from
Draft

Conversation

DechengMa
Copy link
Contributor

@DechengMa DechengMa commented Jul 10, 2020

Implement Sync TestFlight Configuration Command for syncing local and server TestFlight configs.

Support:
  • Create Group
  • Delete Group
  • Modify Group
  • Add Testers to App
  • Add Testers to Group
  • Remove Testers from App
  • Remove Testers from Group

📝 Summary of Changes

Changes proposed in this pull request:

  • Create TestFlightConfigLoader and TestFlightConfiguration
  • Create BetaGroup and BetaTester in new FileSystem module
  • Create SyncResourceComparator for comparing local and server resources
  • Implement push and pull command.

⚠️ Items of Note

Due to the number of API calls, the pull config from server command will take a considerable amount of time.

Limitations:
  • Can't perform creating a group and adding testers to the group at the same time.

🧐🗒 Reviewer Notes

💁 Example

Usage:
asc testflight sync pull [--output-path <output-path>]
asc testflight sync push [--input-path <input-path>] [--dry-run] [--refresh-local]

🔨 How To Test

swift run asc testflight sync pull
swift run asc testflight sync push --dry-run
swift run asc testflight sync push

@DechengMa DechengMa added the enhancement New feature or request label Jul 10, 2020
@DechengMa DechengMa self-assigned this Jul 10, 2020
@DechengMa DechengMa mentioned this pull request Jul 10, 2020
7 tasks
orj
orj previously requested changes Jul 15, 2020
Copy link
Member

@orj orj left a comment

Choose a reason for hiding this comment

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

Looks promising so far.

@DechengMa DechengMa requested a review from orj July 21, 2020 03:38
@DechengMa DechengMa marked this pull request as ready for review July 21, 2020 03:59
@DechengMa DechengMa dismissed orj’s stale review July 21, 2020 03:59

Changes were made upon request

try appsFolder.delete()

try config.forEach {
let appFolder = try appsFolder.createSubfolder(named: $0.app.bundleId!)
Copy link
Member

Choose a reason for hiding this comment

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

Force unwrapping this here seems not ideal. Can an app actually have a nil bundleId? Seems unlikely.

@@ -798,6 +914,54 @@ class AppStoreConnectService {
.await()
}

func pullTestFlightConfigs() throws -> [TestFlightConfiguration] {
let apps = try listApps(bundleIds: [], names: [], skus: [], limit: nil)
Copy link
Member

Choose a reason for hiding this comment

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

We will need to support pulling a subset of apps (filtered by bundleIds probably).

default: "./config/apps",
help: "Path to the folder containing the TestFlight configuration."
) var outputPath: String

Copy link
Member

Choose a reason for hiding this comment

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

We should be able to filter the pulled apps by bundleId.

@Flag(help: "Perform a dry run.")
var dryRun: Bool

func run() throws {
Copy link
Member

Choose a reason for hiding this comment

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

So the fundamental problem with this command is that you're executing the service requests as you're processing. This should be more functional. Eg:

compute(localState, remoteState) -> changeSet
execute(changeSet, isDryRun) -> ()

@DechengMa DechengMa requested a review from orj July 29, 2020 01:18
@DechengMa DechengMa marked this pull request as draft August 26, 2020 06:09
@orj orj removed their request for review April 28, 2021 00:30
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