feat: signinscript - recover from notarization connection error#714
Conversation
hneiva
force-pushed
the
hneiva/rcodesign-retry
branch
from
0197b73 to
72c18e3
Compare
jcristau
left a comment
jcristau
left a comment
There was a problem hiding this comment.
Agree with Ben's comments about keeping the flow the same with/without retries.
| await utils.execute_subprocess(command) | ||
|
|
||
| # Similar implementation to utils.execute_subprocess, but handling some errors: | ||
| message = 'Running "{}"'.format(" ".join(command)) |
There was a problem hiding this comment.
f-string in this case makes it harder to copy-pasta into the terminal :P
|
Updated the code so it's more straight forward, and separates the Still missing tests, I'll add that next, but code is functional. |
|
Fixed and added unit tests. Ready for full review 🚀 |
| if stderr: | ||
| # Unfortunately a lot of outputs from rcodesign come out to stderr | ||
| log.warn(stderr) | ||
| output_lines.append(stderr) |
There was a problem hiding this comment.
I agree with @jcristau's earlier comments about mixing stdout and stderr (this is just another form of that). Presumably you know whether it's stdout or stderr that has the string you're looking for, so you should be able to keep these separate?
There was a problem hiding this comment.
@jcristau comment was regarding sending stderr=asyncio.subprocess.STDOUT to asyncio.create_subprocess_exec - which wouldn't allow us to collect them at all.
Regardless, as stated on the comment above the log.warn, rcodesigndoesn't only output errors to stderr (there's good data in it). Therefore separating them has the opposite desired effect.
Let me know if the inline comment isn't enough.
| args=[path], | ||
| attempts=ATTEMPTS, | ||
| retry_exceptions=RCodesignError, | ||
| ) |
There was a problem hiding this comment.
Are you sure we need to call all of these? The rcodesign docs seems to suggest that your first function should be waiting and stapling all by itself. It says: "Or to wait and automatically staple the file if notarization was successful:"
rcodesign notary-submit \
--api-key-path ~/.appstoreconnect/key.json \
--staple \
path/to/file/to/notarize
It seems like the first call being retried ought to do everything we need (at the cost of resubmitting entirely on any failure).
I think a better way to approach this may be:
- Submit without
--staple, so thatnotarizereturns right away. This should be retried because the submission may fail - Run
notary_waitwith retries (obviously the polling may fail for various reasons). - Run
staple. There shouldn't be any need to retry here.
What do you think? Am I overlooking something?
There was a problem hiding this comment.
staple does reach out to the Apple servers to collect the ticket, therefore I see the benefit of having a retry. But I can change it if you still think it's a good idea.
There was a problem hiding this comment.
OK, good to know. I think the overarching comment still remains though: the first command here (notary-submit --staple) already does all of: submission, polling, stapling. If we're already retrying that command, what is the purpose of running notary_wait and staple separately?
There was a problem hiding this comment.
The way I changed it "wait and staple" is optional. In this case, it just returns the submission id, then we have the wait, and then stapling.
# function:
async def rcodesign_notarize(app_path, creds_path, staple=False):
.......
# usage:
submission_id = await retry_async(
func=rcodesign_notarize,
args=(path, creds_path), # <--- no staple
So, the first command only submits and returns the submission id.
I think you missed the change in the notary-submit code 🙃
There was a problem hiding this comment.
Oh right, sorry. I somehow glossed over the fact that --staple was optional, heh. Alright, proceed!
4 participants
No description provided.