-
Notifications
You must be signed in to change notification settings - Fork 18
Implement Sync Beta Group Commands #147
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
Conversation
8e881e4 to
e822f13
Compare
Sources/AppStoreConnectCLI/Services/Operations/ListBetaTestersOperation.swift
Outdated
Show resolved
Hide resolved
adamjcampbell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some preliminary suggestions and thoughts
Looking pretty good! This may go through some designing though
|
|
||
| struct BetaGroup: TableInfoProvider, ResultRenderable, Equatable { | ||
| struct BetaGroup: TableInfoProvider, ResultRenderable { | ||
| let id: String? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for BetaGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An optional id is for creating a beta group. When creating a new group we don't have the id yet. we can basically create a file with just minimal information (eg. group name, link enabled, etc) and push back
| let publicLinkLimit: Int? | ||
| let publicLinkLimitEnabled: Bool? | ||
| let creationDate: String? | ||
| var testers: String? // CSV file path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where things can get interesting.. Having variables that will populated when pulled from disk but not populated when retrieved from the network
| static func == (lhs: BetaGroup, rhs: BetaGroup) -> Bool { | ||
| return lhs.id == rhs.id && | ||
| lhs.groupName == rhs.groupName && | ||
| lhs.publicLinkEnabled == rhs.publicLinkEnabled && | ||
| lhs.publicLinkLimit == rhs.publicLinkLimit && | ||
| lhs.publicLinkLimitEnabled == rhs.publicLinkLimitEnabled | ||
| } | ||
|
|
||
| func hash(into hasher: inout Hasher) { | ||
| hasher.combine(id) | ||
| hasher.combine(groupName) | ||
| hasher.combine(publicLinkEnabled) | ||
| hasher.combine(publicLinkLimit) | ||
| hasher.combine(publicLinkLimitEnabled) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could potentially only use the id for matching.. do we ever foresee having duplicates?
Did you have to override the default implementations because of the testers file path? There may also be a different solution here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very true. for now, this is only for comparing the BetaGroup object difference, not for testers. when doing something like Set(localGroups).subtracting(serverGroups) I can get the difference and update the server by local
Sources/AppStoreConnectCLI/Services/Operations/UpdateBetaGroupOperation.swift
Outdated
Show resolved
Hide resolved
Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PushBetaGroupsCommand.swift
Outdated
Show resolved
Hide resolved
e822f13 to
986827a
Compare
986827a to
fe92a6c
Compare
|
@DechengMa sorry I've not reviewed this. I'd like to try building a workflow based on this work to test it out. |
Sure no problem. Please PM directly if there's anything that I can do to help. And I'll be around for fixing any bugs / unexpected behaviours in the first place. |
Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PullBetaGroupsCommand.swift
Outdated
Show resolved
Hide resolved
Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/PullBetaGroupsCommand.swift
Outdated
Show resolved
Hide resolved
Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/SyncBetaGroupsCommand.swift
Outdated
Show resolved
Hide resolved
Sources/AppStoreConnectCLI/Commands/TestFlight/BetaGroups/Sync/SyncBetaGroupsCommand.swift
Outdated
Show resolved
Hide resolved
|
|
||
| extension BetaGroup: FileProvider { | ||
| var fileName: String { | ||
| "\(app.bundleId ?? "")_\(groupName).yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be normalising the filename a bit better here. groupName could be an unsafe filename. We should turn all non alpha numeric characters into underscores.
extension String {
func filenameSafe() -> String {
let unsafeFilenameCharacters = CharacterSet(charactersIn: " *?:/\\.")
return str.components(separatedBy: unsafeFilenameCharacters).joined(separator: "_")
}
}I'm working with an app that has a beta group named "a/h_Security" so this code creates a subdirectory named "bundleId_a" with a file in it called "h_Security.yml"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an account that has a lot of groups it would be better to nest them in subdirectories. The app.bundleId could be a subdirectory rather than a prefix.
Also, surely the app.bundleId will be non-nil? When will it ever be nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually thinking about he normalised/safe filename some more I think when group names are translated to file/path names they should be really cleaned up:
Eg: A group name like d_App Team (Learn & Adopt) should be translated to d-app-team-learn-adopt. The problem with that however is that removing characters might mean there could be clashes. We might have to come up with some resolution strategy in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to suggest here is that the filenames should be names that a developer might choose if they were authoring them themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work @DechengMa but I think we need to refine it a bit.
The current pulled structure is very flat and includes redundant data. I think we should change the structure so it is like this:
- config/betagroups/
- com.example.appid/
- safe-group-name/
- betagroup.yml
- betatesters.csv
- safe-group-name/
- com.example.appid/
|
Something else I'd like us to clean up here is the duplication of information. When we pull down the details of the beta group the |
46aa03a to
0e381c0
Compare
|
a new redesigned sync command will be implemented in #203 |
Implement basic sync beta group command.
📝 Summary of Changes
Changes proposed in this pull request:
let id: String?field to Beta Group model. I know we don't want to do this but it's a must coz two groups can have the same name.var testers: [String]to Beta Group for storing tester emailsBetaGroupextend toEquatable, Hashablefor comparing inSet, toSyncResultRenderablefor rendering dry run and sync result, toFileNameGettablefor generating a beatified filename (a String extension was made for snakeCased filename).SyncResultRendererfor printing sync and dry run resultenum SyncStrategyfor both rendering result and handle the update.protocol ResourceFolderManagerandBetaGroupFolderManager, for managing folders and group files.ListBetaTestersOperationand createUpdateBetaGroupOperation. Create service functionpullBetaGroups,updateBetaGroup,deleteBetaGroup.SyncBetaGroupsCommandwhich containsPullBetaGroupsCommandandPushBetaGroupsCommand.🧐🗒 Reviewer Notes
💁 Example
Usage:
asc testflight betagroup sync pull [--output-path <output-path>]asc testflight betagroup sync push [--input-path <input-path>] [--dry-run]--output-pathand--input-pathis a folder path.CLI will go through all the files in that folder and find file with a suffix of
ymland read it as a beta group file.Sample Output:
TODO
Sample file structure
TODO
🔨 How To Test
asc testflight betagroup sync pullasc testflight betagroup sync push --dry-runasc testflight betagroup sync pushHow to
Note: Please run
pullfirst to get server groups and testers before run pushCreate a new group via sync:
./config/betagroups(or the input folder path), suffix must beymlasc testflight betagroup sync pushDelete a group
asc testflight betagroup sync pushUpdate a group
fields that support editing: (edit other fields will not work)
asc testflight betagroup sync push