Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[deliver][pilot] use altool instead of using iTMSTransporter for Xcode 14 #20631
[deliver][pilot] use altool instead of using iTMSTransporter for Xcode 14 #20631
Changes from 35 commits
83cdc08
ec4cfee
f1e0575
c3a66db
39c919e
74f681d
6d10343
ce9b12d
40337e3
0f2a3d4
d675551
fa388c7
dcb97a8
5bd7d3f
2023aa8
7a13c48
8df9559
ed28c90
375f951
711eb3c
747178f
13afc8c
d36bf23
e4e44ac
4f677a6
9988e33
4ebf009
ddb437a
b4f5324
44bf7aa
01a5873
7490c5c
388726b
b070b58
1cdc817
c82e99a
bdbacb5
00c974c
1994860
16249e7
8ab40fa
22cfcec
3fc11fa
980f189
669bebc
98e1a88
355cc9d
0f6bfcb
f4cd020
95e7cb8
38f23ee
30839b9
a2ea2fb
874320c
ab02d6d
71e57d6
b518207
8d5bb62
5beca1e
4f4d3ee
cd2c504
4b93a81
18f460f
ac7358d
07181a7
599ab2b
8be9c7e
8e7d6f3
c005aae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'd love for this method to use named parameters at some point (
build_upload_command(username:, password:, source: "/tmp", provider_short_name: "", jwt: nil, platform: nil, api_key: nil)
for easier-to-read call sites; but that's just a code-style nitpick and not a blocker at all, and can be ironed out in a separate PR, so no worries.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 see, since
build_upload_command
's style is same with other executer class so it would be kind of refactoring. I think I can do it later PR 🙇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.
Totally 👍 no need to fix this in this PR given the refactoring it will involve indeed. And not that critical if we don't do it anyway to be honest 🙃
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.
Of course if you decide to go with my suggestion of using
TempFile
to generate the temporary p8 file, you'll have to adjust this here too.Easiest way to do this imho is to pass the
tmp_p8_file_path
that you'd generate viaTempFile
(orDir.mktmpdir
) as theapi_key
parameter of thebuild_upload_command
function at this call site, instead of passing the actual API key value (since that value would already be written to the temp p8 file and we don't use it directly in this method's implementation anyway). And then use"API_PRIVATE_KEYS_DIR=#{File.dirname(api_key)}" if use_api_key
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.
Other API key value is necessary like
key_id
andissuer_id
, so I wanna you this api_key argument as current actual API key valueThere 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.
Code-style nitpick: merging those two lines into a single one using a ternary condition will make this easier to read/reason about imho:
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'd suggest to instead use
Dir.mktmpdir
to create an intermediate and guaranteed-unique temporary folder and generate the key file inside it. Or useTempFile
to create the file.(PS: In either case you can provide a prefix (like
deliver-
) to use for the autogenerate unique filename which could help identify it on disk… even though it'll get deleted soon after so it doesn't really matter)If you go that route you'll have to declare
tmp_p8_file_path = nil
before thebegin
, then set it to a value on this line, so that you'll be able to use the sametmp_p8_file_path
value in yourensure
block to delete it.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.
Done! c82e99a
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.
If you go with my suggestion above to use
TempFile
to create the file, this would become just: