Skip to content

Appstoreconnect#54

Closed
dvc94ch wants to merge 5 commits into
indygreg:mainfrom
dvc94ch:appstoreconnect
Closed

Appstoreconnect#54
dvc94ch wants to merge 5 commits into
indygreg:mainfrom
dvc94ch:appstoreconnect

Conversation

@dvc94ch

@dvc94ch dvc94ch commented Dec 12, 2022

Copy link
Copy Markdown
Collaborator

Moves the appstoreconnect api code into it's own crate and extends it with a cli tool and support for the devices, certificates, bundle ids and profiles apis. To generate a key and certificate you can use the following command:

asconnect generate-key --api-key path.to.unified.api.key --type development output.key.and.certificate.pem

This was referenced Dec 12, 2022
Closed
Closed
Closed
Merged
@dvc94ch dvc94ch requested review from indygreg and roblabla December 12, 2022 12:45
indygreg
indygreg reviewed Dec 17, 2022
View reviewed changes

@indygreg indygreg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I glanced at this and am definitely supportive. I recently refactored the app store connect code into its own modules suspecting that we'd grow support for more APIs.

Are you receptive to changing the crate name? asconnect isn't obvious to me. How about app-store-connect?

I have a preference for not using anyhow in the library part of the new crate because it makes granular error handling harder. Are you receptive to preserving the use of thiserror? We don't have to use granular error variants - I just think out of principle we want the ability to exposure granular errors for some failures.

indygreg added a commit that referenced this pull request Dec 18, 2022
@indygreg
apple-codesign: remove module declarations
0ef5364 
We don't need to define these in `main.rs`.

Patch inspired by #54.
@dvc94ch dvc94ch force-pushed the appstoreconnect branch 2 times, most recently from 20fded8 to 20ad8db Compare December 18, 2022 18:23
@dvc94ch

dvc94ch commented Dec 18, 2022

Copy link
Copy Markdown
Collaborator Author

so I refactored the error handling. you're right that the previous error handling could be considered sloppy. we have 4 different error types now:

  • InvalidPemPrivateKey
  • MissingApiKey
  • AppStoreConnectError
  • NotarizationError

This allows fine grained error handling without wrapping errors, missing backtraces, etc. And is what I'd consider good error handling. Let me know what you think.

@dvc94ch

dvc94ch commented Dec 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

to clarify what I mean. the error type is always anyhow::Result ensuring backtraces are captured properly. the source of an error is always a concrete type that can be matched on using downcast_ref::<T> and additional context is added with context instead of putting it in yet another nested enum. the reason I'm skeptical of a gigantic error enum being a good idea is because unless your library only has a single public function, many of the errors in the enum can never actually occur in most functions anyway. The second reason is the insistence on the rust ecosystem on glorifying error handling which does not provide or preserve adequate backtraces. If someone writes a blog post titled "why backtraces are a thing of the past" providing a good argument for it I might change my mind, but as far as I can tell this anti backtrace consensus is not on solid ground.

@indygreg

indygreg commented Dec 18, 2022

Copy link
Copy Markdown
Owner

I'm fine with this hybrid error approach. Agree that monolithic errors are not great and that backtraces don't need to be preserved at all costs. Will look at patch today.

indygreg
indygreg reviewed Dec 18, 2022
View reviewed changes

@indygreg indygreg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is on the right track. I don't want to step too much in the way of progress.

Let's fix the missing license headers and the new clippy warnings and merge what we have.

As for the license, feel free to convert the new crate to Apache 2.0 + MIT (like other crates) if you want. Pretty sure it is just the two of us who authored all the migrated code.

Comment thread app-store-connect/src/bundle_api.rs
Comment thread app-store-connect/src/bundle_api.rs Outdated
Comment thread app-store-connect/src/bundle_api.rs
Comment thread app-store-connect/src/certs_api.rs Outdated
Comment thread app-store-connect/src/certs_api.rs Outdated
Comment thread app-store-connect/src/main.rs Outdated
Comment thread app-store-connect/src/main.rs
Comment thread app-store-connect/src/main.rs Outdated
Comment thread app-store-connect/src/main.rs Outdated
Comment thread apple-codesign/src/lib.rs Outdated
@dvc94ch

dvc94ch commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator Author

Think this one is good to go

indygreg
indygreg reviewed Dec 19, 2022
View reviewed changes

@indygreg indygreg left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I applied this locally and made a handful of modifications (mostly to add missing license files, a changelog, etc) and was about to push it.

But the CLI is completely broken:

$ cargo run --bin app-store-connect
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s
     Running `target/debug/app-store-connect`
thread 'main' panicked at 'Command app-store-connect: Global arguments cannot be required.

        'api_key' is marked as both global and required', /home/gps/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-4.0.29/src/builder/debug_asserts.rs:258:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I pushed my local branch to the app-store-connect branch. Do you want to fix things before merge or after?

If before, just hard reset your branch to the one I pushed and update this PR. If after, I can push that commit to main whenever.

@dvc94ch

dvc94ch commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator Author

Shit, was thinking I had to test the global change, but then forgot about it. Thanks for noticing.

@dvc94ch

dvc94ch commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator Author

@indygreg I think the problem fixed itself with you dependency update. I rebased on main and it all worked fine.

@dvc94ch

dvc94ch commented Dec 19, 2022

Copy link
Copy Markdown
Collaborator Author

my mistake. the error message changed to not recognizing the provided arg, didn't fix the problem. Fixed now.

@indygreg indygreg closed this in f39ee46 Dec 20, 2022
@indygreg

indygreg commented Dec 20, 2022

Copy link
Copy Markdown
Owner

I cherry picked the 2 commits you pushed since the last time I looked at things and then pushed the result.

I'm willing to push a release to crates.io whenever you are. Just let me know by filing a GitHub issue.

@dvc94ch dvc94ch deleted the appstoreconnect branch December 20, 2022 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@indygreg indygreg indygreg left review comments

@roblabla roblabla Awaiting requested review from roblabla

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dvc94ch @indygreg