Handle potential errors during new deployment flow

[dotNomad]

This PR fixes two things:

1. When an type="unknown" deployment is created the entrypoint could be blank when we reach normalizeConfig, which would cause the entrypoint field to be missing from the Configuration and the files array to only contain a /
2. When we create a new deployment / configuration combo, if the Configuration we create is invalid (read type="unknown") the API calls to update the file lists with the Posit deployment record / configuration file were failing

Why were the APIs failing?

The API calls were failing because when we get a Config from a file we validate said file before decoding it:

publisher/internal/config/config.go

Line 63 in fa07fcf

err := ValidateFile(path)

This makes sense, we have to have a valid file to get a valid Config, but the strict nature of not allowing type: "unknown" means our config creation here

publisher/internal/inspect/detectors/all.go

Line 79 in 66fdddb

allConfigs = append(allConfigs, newUnknownConfig())

causes the following API call to fail.

We have some ways we get around this in others parts of the code:

publisher/internal/services/api/put_configuration.go

Line 80 in 8b382de

// We permit configurations with `unknown` type to be created,

It would be ideal, when we don't care about the type, to be able to ignore that part of our validation. Or even better allow the unknown type completely until it matters.

Intent

Resolves #2419

Type of Change

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

Approach

The approach here was two fold.

The backend now produces a more correct configuration in cases where type = 'unknown'.

The frontend also handles the API calls when creating a new deployment separately, only bailing out when absolutely necessary - like when a file fails to be created.

The tradeoff here is that type = 'unknown' configurations will not have the .posit files listed in their files field - at least with the way it is currently implemented.

Automated Tests

A test for the backend has been added.

[marcosnav]

Tried the current state of this PR, maybe you already noticed it but want to raise that a panic on the go code is still happening, a bit buried beneath output logs.

We should fix that, seems to be because of the configuration not being returned after validation at post_config_files.go. Following the conversation from Slack, treating content type unknown as a warning and not a reason to invalidate the configuration might help with this an unblock flows that could be productive to the user.

[marcosnav]

I'm not 100% on board with ignoring errors. We maybe can ignore specific known errors when we know it's ok, but there could also be surprising unknown errors landing catch blocks that should be noticed.

[dotNomad]

That is totally fair. I think in this case because these operations aren't completely blocking on the rest of the config + deployment file creation any error we get, even if it is unexpected, shouldn't stop the rest of the function from continuing.

I have two follow-up questions:

• do you agree with the above?
• what do you propose we add to the catch?

The most obvious addition could be some logging, but I didn't think showing a notification to the user or something that heavy was necessary for a non-blocking action failing.

[marcosnav]

Yes, logging might be the best option here. Maybe also some more details in code comments about why this isn't and shouldn't be a blocking operation, this to help future collabs to better determine if this needs to change in case something else lands within the try block.

[dotNomad]

Included some debug logging in 5241816

I also improved the comment a bit to be more explicit about why it is fine to continue on in the new deployment flow.

[jonkeane]

The tradeoff here is that type = 'unknown' configurations will not have the .posit files listed in their files field - at least with the way it is currently implemented.

Could you say more about this + what the practical impact is on users who fall into this circumstance? I think it's ok / an improvement, but I want to make sure we think about that and confirm it.

[dotNomad]

Could you say more about this + what the practical impact is on users who fall into this circumstance? I think it's ok / an improvement, but I want to make sure we think about that and confirm it.

Absolutely. The impact is deployments initially made with the unknown type won't have the generated config with .posit files.

This is due to the API calls to include the files failing because type: 'unknown' makes the file fail validation prior to decoding.

To the point @marcosnav made we can change the behavior and avoid this tradeoff.

Part of the reason I mentioned it as happening "in this current state" was to avoid going down a rabbit hole of fixing several bugs / weird behaviors at once without opening it up to some discussion.

[jonkeane]

Could you say more about this + what the practical impact is on users who fall into this circumstance? I think it's ok / an improvement, but I want to make sure we think about that and confirm it.

Absolutely. The impact is deployments initially made with the unknown type won't have the generated config with .posit files.

This is due to the API calls to include the files failing because type: 'unknown' makes the file fail validation prior to decoding.

To the point @marcosnav made we can change the behavior and avoid this tradeoff.

Part of the reason I mentioned it as happening "in this current state" was to avoid going down a rabbit hole of fixing several bugs / weird behaviors at once without opening it up to some discussion.

Sorry, I meant this from the perspective of our users. Correct me if I'm wrong on anything here, but I was hoping for something like: we want to encourage folks to include their config + deployment files in what they publish to connect so we include them in the files section of the config (but folks can decide not to if they want). But in this situation, for whatever reason these files aren't being included. This makes it so that the deployment is still possible (to be attempted at least), we just degrade the behavior where the config + deployment files aren't included in the bundle. That's less good for us (we believe people should ~always include those files), but it's not blockingly detrimental to our users since they are able to proceed with their deployment (likely see that it fails, and then fix the issue and then actually proceed).

[dotNomad]

we want to encourage folks to include their config + deployment files in what they publish to connect so we include them in the files section of the config (but folks can decide not to if they want). But in this situation, for whatever reason these files aren't being included. This makes it so that the deployment is still possible (to be attempted at least), we just degrade the behavior where the config + deployment files aren't included in the bundle. That's less good for us (we believe people should ~always include those files), but it's not blockingly detrimental to our users since they are able to proceed with their deployment (likely see that it fails, and then fix the issue and then actually proceed).

This is all correct. The extra bits I would emphasize are:

• the type = 'unknown' config is invalid so while it doesn't stop deploying it does mean the sidebar views aren't visible so to fix the .posit files missing the user would have to
  • fix the config by choosing a valid type
  • manually edit the config to include the .posit files

To be clear this PR is the most minimal fix to #2419. My hope is to either add to this PR, or follow it up and fix this so .posit files are always included.

See this conversation in Slack for a bit more: https://positpbc.slack.com/archives/C05D7NZD52S/p1731025235725539

[jonkeane]

the type = 'unknown' config is invalid so while it doesn't stop deploying it does mean the sidebar views aren't visible so to fix the .posit files missing the user would have to

Ah yes, that's definitely good to highlight / would appear as an at least semi-broken / weird state for most users.

To be clear this PR is the most minimal fix to #2419. My hope is to either add to this PR, or follow it up and fix this so .posit files are always included.

nods yeah, minimal is great! And I'm not trying to push us to not do that — just wanted to confirm that we are doing a minimal thing that also doesn't make our user's end experience worse (and be explicit about that + what those things are).