feat: Pre-flight validate the entrypoint file exists

[dotNomad]

Adds a pre-flight check to validate that the entrypoint file, specified in the configuration, exists prior to deploying to Connect.

Intent

Resolves #2964

Type of Change

• • Bug Fix
• • New Feature
• • Breaking Change
• • Documentation
• • Refactor
• • Tooling

Approach

Added checkEntrypoint validation alongside where we do checkRequirementsFile.

It does skip validation on any module references, which happens with Shiny Express. The entrypoint looks like shiny.express.app:app_2e_py in that case so entrypoints containing : are skipped.

I skipped them rather than attempting to see if that file exists or refactoring the order in which we swap out the EntrypointFile. To swap in the module reference for Shiny Express, the file has to be read - so we know it exists already.

User Impact

Users will now receive an immediate, clear error message when attempting to deploy with a missing entrypoint file, rather than having the deployment fail on the server.

Automated Tests

Unit tests were added for the new functionality 🎉

Directions for Reviewers

1. Create a config with a non-existent entrypoint file
2. Attempt to deploy
3. Verify an early failure occurs with a clear error message about the missing entrypoint

Checklist

• I have updated the root CHANGELOG.md to cover notable changes.

[jonkeane]

I'm a soft -1 on making this change be a hard-stop error for two reasons (1) we don't have an actionable way for someone to resolve this (2) if we get any of the detection wrong, we prevent people from deploying. While these deployments aren't in a good state, they do actually function (e.g. if you have a static website that you send up, say that index.html is the entrypoint, but only send up a few other html files, you can get to them at the standalone URL

As for (2):
In the original PR, one suggestion was:

Maybe for static content in general, not just Quarto rendered content, we could ping after deployment and eval that it is a 2XX and not 4XX? The warn the user? 🤔 (thinking out loud here, ideas are welcome)

But even without doing a check for a 2XX, we could make this be a warning (or even an error) at the end of the deploy rather than forcing a positive stop in the deploy process. That way someone has a way out of this and aren't stuck.

as for (1):
We don't surface the entrypoint (or being able to change it) anywhere in the UI. If all that needs to happen is add the entrypoint to the files list, that's easy enough. But if it's something else (like they need to specify a different entrypoint) for most folks going into the TOML and editing by hand is not what they are expecting + we don't give a good on ramp to that. IMO if we're going to make someone correct this before they move forward, we need to have all the necessary bits available in the UI first. And, even then there is still some of the concern of (2) that stands.

[dotNomad]

Thinking through your feedback @jonkeane

On point 1

Great points. Do you think its beneficial to use all the same tooling here to warn (rather than fail) that the entrypoint in the configuration doesn't exist on the system?

On point 2

Maybe for static content in general, not just Quarto rendered content, we could ping after deployment and eval that it is a 2XX and not 4XX? The warn the user? 🤔 (thinking out loud here, ideas are welcome)

We left 404 and 405 open as non-errors since, for some content, that is a valid response for the initial page served by Connect. (An API that only non-GET handlers for example).

We should be able to use the Content configuration to determine its type and have additional logic if its static since that should always be a 2xx response (if valid).

Thanks for re-upping that comment

publisher/internal/clients/connect/client_connect.go

Lines 478 to 499 in 785e085

	func (c *ConnectClient) ValidateDeployment(contentID types.ContentID, log logging.Logger) error {
	url := fmt.Sprintf("/content/%s/", contentID)
	log.Info("Testing URL", "url", url)
	_, err := c.client.GetRaw(url, log)
	agentErr, ok := err.(*types.AgentError)
	if ok {
	httpErr, ok := agentErr.Err.(*http_client.HTTPError)
	if ok {
	if httpErr.Status >= 500 {
	// Validation failed - the content is not up and running
	return types.NewAgentError(types.ErrorDeployedContentNotRunning, errValidationFailed, nil)
	} else {
	// Other HTTP codes are acceptable, for example
	// if the content doesn't implement GET /,
	// it may return a 404 or 405.
	return nil
	}
	}
	}
	return err
	}

[jonkeane]

On point 1
Great points. Do you think its beneficial to use all the same tooling here to warn (rather than fail) that the entrypoint in the configuration doesn't exist on the system?

Yeah, that would be fine. We still have the problem of "if the entrypoint field is wrong how are you going to fix it" but at least it doesn't prevent someone for using the publisher entirely.

On point 2
Maybe for static content in general, not just Quarto rendered content, we could ping after deployment and eval that it is a 2XX and not 4XX? The warn the user? 🤔 (thinking out loud here, ideas are welcome)

We left 404 and 405 open as non-errors since, for some content, that is a valid response for the initial page served by Connect. (An API that only non-GET handlers for example).

We should be able to use the Content configuration to determine its type and have additional logic if its static since that should always be a 2xx response (if valid).

This might actually be FUD/YAGNI actually. For Plumber, FastAPI, and FlaskAPIs connect will actually redirect / check for docs paths in order for get / to return something meaningful. There might be edge cases somewhere where someone truly disables these, but out of the box, we go our of our way to make sure there's something there. But regardless, like you say: we could also do this for static content only since that is a more direct sign of life.

Thanks for re-upping that comment