Add logs url on failure

[jonkeane]

Intent

Add the logs URL to the error message so that folks find the logs easier.

Resolves: #636

Type of Change

• Bug Fix
• New Feature
• Breaking Change

Approach

Fetch and add the logs URL to the error message

Automated Tests

None were broken or added.

Directions for Reviewers

Output from testing this locally:

% rsconnect deploy shiny --name radixu .
Validating server... 	[OK]
Validating app mode... 	[OK]
Making bundle ... 	[OK]
Deploying bundle ... 	[OK]
Saving deployed information... 	[OK]
Building Python Shiny application...
...
Launching Python Shiny application...
Deployment completed successfully.
	 Dashboard content URL: https://rsc.radixu.com/connect/#/apps/09bfbb6a-7e7c-4a37-bafd-9a0bd7b4b67c/access
	 Direct content URL: https://rsc.radixu.com/content/09bfbb6a-7e7c-4a37-bafd-9a0bd7b4b67c/
Verifying deployed content... 	[ERROR]: Could not access the deployed content. The app might not have started successfully.
	For more information: https://rsc.radixu.com/connect/#/apps/09bfbb6a-7e7c-4a37-bafd-9a0bd7b4b67c/logs
Error: Could not access the deployed content. The app might not have started successfully.
	For more information: https://rsc.radixu.com/connect/#/apps/09bfbb6a-7e7c-4a37-bafd-9a0bd7b4b67c/logs

Checklist

• I have updated CHANGELOG.md to cover notable changes.
• I have updated all related GitHub issues to reflect their current state.

[jonkeane]

I will add changelog after initial review / confirmation this looks like the direction we want.

[jonkeane]

New line + tab to match the other URLs that get put in the output, but that's not strictly necessary.

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
4834	3587	74%	0%	🟢

New Files

No new covered files...

Modified Files

File	Coverage	Status
rsconnect/api.py	71%	🟢
TOTAL	71%	🟢

updated for commit: 635a71e by action🐍

[nealrichardson]

Output from testing this locally:

Looks like the error message is printed twice? And is indented one space less than the other messages?

[jonkeane]

Looks like the error message is printed twice?

Yeah, it is. I tried to find where/why this was once, but didn't get to the bottom of it and wanted to get this up. I will keep looking though (or at least create an issue to fix it). The duplication is not a result of these changes though.

And is indented one space less than the other messages?

Ah, right indeed. those other messages have \t D and this one has \tF. I'll match the other ones for now (so this change is less disruptive) but think if we're going to \t we shouldn't also space immediately after.

[jonkeane]

Looks like the error message is printed twice?

Yeah, it is. I tried to find where/why this was once, but didn't get to the bottom of it and wanted to get this up. I will keep looking though (or at least create an issue to fix it). The duplication is not a result of these changes though.

Ok, with some more digging, I think this was introduced in #244 and ultimately what happens is the logger is printed to and then it is printed to the console. Though I cannot find any discussion in the PR or linked refactor issues issue about if this is the desired behavior and why.

I would be supportive of (probably in a separate PR) exploring if the cli_exception_handler needs to exist at all.

[tdstein]

The paranoid programmer in me says to avoid the possible KeyError before the exception is raised.

Suggested change

	+ f"\n\t For more information: {self.app_config(app_guid)['logs_url']}"
	+ f"\n\tFor more information: {self.app_config(app_guid).get('logs_url')}"

[jonkeane]

(1) yes, agreed about the key error (2) the there is (sadly) intentional to avoid needing to s/\t /\t/g elsewhere

[jonkeane]

@tdstein any thoughts on:

Ok, with some more digging, I think this was introduced in #244 and ultimately what happens is the logger is printed to and then it is printed to the console. Though I cannot find any discussion in the PR or linked #256 about if this is the desired behavior and why.

I would be supportive of (probably in a separate PR) exploring if the cli_exception_handler needs to exist at all.

[tdstein]

@tdstein any thoughts on:

Ok, with some more digging, I think this was introduced in #244 and ultimately what happens is the logger is printed to and then it is printed to the console. Though I cannot find any discussion in the PR or linked #256 about if this is the desired behavior and why.
I would be supportive of (probably in a separate PR) exploring if the cli_exception_handler needs to exist at all.

This predates me as well. I think a separate PR to remove the duplication is correct.

My guess is that the intent is for logger to write to stderr and for click to write to stdout. In this case, I can see why a log message and print call would be necessary. But, I think it's overkill for this package. It should either use click.echo or Python logging.

[jonkeane]

The failures are on main as well, I made #644 for those.