Skip to content

Use new CDN for dotnet builds - #1230

Merged
Eljo George (eljog) merged 3 commits into
devcontainers:mainfrom
sliekens:dotnet-cdn
Jan 9, 2025
Merged

Use new CDN for dotnet builds#1230
Eljo George (eljog) merged 3 commits into
devcontainers:mainfrom
sliekens:dotnet-cdn

Conversation

@sliekens

@sliekens Steven (sliekens) commented Dec 28, 2024

Copy link
Copy Markdown
Contributor

The azureedge.net domain might stop working as detailed in dotnet/core#9671

Changes in this PR:

Additional context

@sliekens

Steven (sliekens) commented Dec 28, 2024

Copy link
Copy Markdown
Contributor Author

@chrmarti Christof Marti (chrmarti) left a comment

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.

Great, thanks for the PR! Left a few questions.

Comment thread src/dotnet/scripts/vendor/dotnet-install.sh
Comment thread src/dotnet/scripts/vendor/dotnet-install.sh
@sliekens

Steven (sliekens) commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Christof Marti (@chrmarti) the dotnet-install.sh script is copied from https://github.com/dotnet/install-scripts/blob/main/src/dotnet-install.sh so I'm not qualified to answer those questions but I think Matt Mitchell (@mmitche) can help.

(In any case, fixes would have to be done over there, as there is a github action to overwrite the copy in this repo with upstream changes.)

@mmitche

Copy link
Copy Markdown

Yeah, the install script is going to get another update soon, removing the use of the azureedge domains, at least outside of sanitization. So the script would only try to access the new domains. We're not quite sure yet when we will update old aka.ms links. Once they are updated, then we would remove the sanitization altogether.

@chrmarti

Copy link
Copy Markdown
Contributor

Matt Mitchell (@mmitche) Great, this sounds like we should wait for that imminent update of the install script?

Updating the aka.ms redirects would smooth things out I guess, is there a reason to hold back on that?

@sliekens

Steven (sliekens) commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Don't forget that script is updated here automatically every Sunday, e.g. see #1236

I just realized I forgot to bump the version 🤔. Not sure about the semantics of this change either. Probably not a major bump, the behavior didn't change that much and usage remains the same. On the other hand, this change could break users behind corporate firewalls where the CDN is not allowed.

@chrmarti

Copy link
Copy Markdown
Contributor

Don't forget that script is updated here automatically every Sunday, e.g. see #1236

I just realized I forgot to bump the version 🤔. Not sure about the semantics of this change either. Probably not a major bump, the behavior didn't change that much and usage remains the same. On the other hand, this change could break users behind corporate firewalls where the CDN is not allowed.

Minor (or patch) might work best since that will be picked up when the devcontainer.json only specifies the major version. There is also an optional lockfile devcontainer-lock.json that pins the exact feature version and checksum and would hold the update back.

I guess we could just run the workflow you mention then (catching up on how we do this): https://github.com/devcontainers/features/actions/workflows/update-dotnet-install-script.yml

@sliekens

Copy link
Copy Markdown
Contributor Author

A new minor version makes the most sense to me, I'll do that. I can revert the changes to dotnet-install.sh vendor script in this PR if desired, but the changes in the dotnet-helpers.sh script are still needed, I wrote those scripts specifically for the devcontainer feature to find the latest version no matter the STS or LTS status.

@sliekens

Copy link
Copy Markdown
Contributor Author

I guess I should fix the failing test while I'm at it 🤔.

- add .net9.0 test case
- remove 'build and run' from scenarios with dynamic runtime versions because they fail when the version changes
@sliekens

Steven (sliekens) commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Christof Marti (@chrmarti) I think this work is complete now.

Final changes:

  • Fixed failing scenarios which installed the latest dotnet version (now .NET 9) but then tried to run a .NET 8 project
  • Added .NET 9 to one of the scenarios where multiple versions are tested all at once
  • Bumped dotnet minor version
  • Bumped oryx minor version

image

@chrmarti

Copy link
Copy Markdown
Contributor

Steven (@sliekens) Looks great, thanks! We can update the script again when the original changes later this/next week. Adding Gaurav Saini (@gauravsaini04) as a second reviewer.

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.

All the changes done, LGTM 👍🏻
Thanks Steven (@sliekens) !

@eljog

Copy link
Copy Markdown
Member

We can update the script again when the original changes later this/next week

SGTM, Gaurav Saini (@gauravsaini04) let's make a note of that followup

@eljog
Eljo George (eljog) merged commit 978aa3f into devcontainers:main Jan 9, 2025
@sliekens
Steven (sliekens) deleted the dotnet-cdn branch January 9, 2025 18:26
@cetinbug

Copy link
Copy Markdown

For our Azure Functions, we currently use the URL functionscdn.azureedge.net to retrieve extension bundles. However, after reviewing the issue, I noticed that there is no specific mention of the subdomain functionscdn in the migration details. Could you please clarify if this subdomain will also be migrated and, if so, to which domain?

For example, we are getting extension bundles from a url like: "https://functionscdn.azureedge.net/public/ExtensionBundles/Microsoft.Azure.Functions.ExtensionBundle.Preview/$EXTENSION_BUNDLE_VERSION"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants