Skip to content

Handle missing settings.json for Codespaces/Devcontainers#6880

Merged
mitchdenny merged 2 commits intomainfrom
mitchdenny/create-settings-file
Dec 7, 2024
Merged

Handle missing settings.json for Codespaces/Devcontainers#6880
mitchdenny merged 2 commits intomainfrom
mitchdenny/create-settings-file

Conversation

@mitchdenny
Copy link
Copy Markdown
Member

@mitchdenny mitchdenny commented Dec 6, 2024

… handle missing settings.json file.

Description

This pull request includes several changes to improve handling of what happens when the machine-wide settings.json file isn't present. The solution is to detect if the file exists and if it isn't - create it. This required a bunch of other collateral changes which are probably good long-term changes towards improving testability (although I still haven't tackled that yet).

I've also added Copilot to our contributing devcontainer since we would normally expect this to be present in our local VS installs - it's kinda helpful :)

Fixes #6873

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
Microsoft Reviewers: Open in CodeFlow

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 suggestion.

Files not reviewed (1)
  • .devcontainer/contributing/devcontainer.json: Language not supported

Comment thread src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs Outdated
@mitchdenny mitchdenny added this to the 9.1 milestone Dec 6, 2024
@mitchdenny mitchdenny requested a review from Copilot December 6, 2024 01:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 5 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • .devcontainer/contributing/devcontainer.json: Language not supported
Comments skipped due to low confidence (2)

src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs:86

  • [nitpick] The error message 'Codespaces or Devcontainer not detected.' could be more descriptive. Consider changing it to 'Neither Codespaces nor Devcontainer environment detected. Unable to determine the correct settings file path.'
throw new DistributedApplicationException("Codespaces or Devcontainer not detected.");

src/Aspire.Hosting/Devcontainers/DevcontainerSettingsWriter.cs:90

  • Ensure that the new behavior introduced by 'EnsureSettingsFileExists' is covered by tests.
async Task EnsureSettingsFileExists(string path, CancellationToken cancellationToken)

@mitchdenny mitchdenny force-pushed the mitchdenny/create-settings-file branch from 6797934 to a25f33b Compare December 6, 2024 02:25
await writer.WriteAsync("{}".AsMemory(), cancellationToken).ConfigureAwait(false);
}
}
catch (IOException ex) when (ex.Message == $"The file '{path}' already exists.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am gonna assume you tested this 😄

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did. The race should be pretty rare as well. I was just being cautious.

@mitchdenny mitchdenny merged commit 1be73ac into main Dec 7, 2024
@mitchdenny mitchdenny deleted the mitchdenny/create-settings-file branch December 7, 2024 05:09
@github-actions github-actions Bot locked and limited conversation to collaborators Jan 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Devcontainer port forwarding logic blows up if settings.json not present.

3 participants