Skip to content

Use new CDN for dotnet builds - #1230

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

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

Conversation

@sliekens

@sliekens 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

sliekens commented Dec 28, 2024

Copy link
Copy Markdown
Contributor Author

@bamurtaugh
bamurtaugh requested a review from chrmarti January 6, 2025 17:39

@chrmarti 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

sliekens commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

@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 @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

mmitche commented Jan 7, 2025

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

chrmarti commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

@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

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

chrmarti commented Jan 7, 2025

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

sliekens commented Jan 7, 2025

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

sliekens commented Jan 7, 2025

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

sliekens commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

@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

@sliekens
sliekens requested a review from chrmarti January 7, 2025 22:12
@chrmarti
chrmarti requested a review from gauravsaini04 January 8, 2025 07:07
@chrmarti

chrmarti commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

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

@gauravsaini04 gauravsaini04 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.

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

@eljog

eljog commented Jan 8, 2025

Copy link
Copy Markdown
Member

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

SGTM, @gauravsaini04 let's make a note of that followup

@eljog
eljog merged commit 978aa3f into devcontainers:main Jan 9, 2025
@sliekens
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