Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new CDN for dotnet builds #1230

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Conversation

sliekens
Copy link
Contributor

@sliekens sliekens commented Dec 28, 2024

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

Changes in this PR:

Additional context

@sliekens
Copy link
Contributor Author

sliekens commented Dec 28, 2024

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

@chrmarti chrmarti left a comment

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.

src/dotnet/scripts/vendor/dotnet-install.sh Show resolved Hide resolved
src/dotnet/scripts/vendor/dotnet-install.sh Show resolved Hide resolved
@sliekens
Copy link
Contributor Author

sliekens commented Jan 7, 2025

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

mmitche commented Jan 7, 2025

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

chrmarti commented Jan 7, 2025

@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
Copy link
Contributor Author

sliekens commented Jan 7, 2025

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

chrmarti commented Jan 7, 2025

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
Contributor Author

sliekens commented Jan 7, 2025

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
Contributor Author

sliekens commented Jan 7, 2025

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
Copy link
Contributor Author

sliekens commented Jan 7, 2025

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

chrmarti commented Jan 8, 2025

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

Copy link
Contributor

@gauravsaini04 gauravsaini04 left a comment

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
Copy link
Member

eljog commented Jan 8, 2025

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
18 checks passed
@sliekens sliekens deleted the dotnet-cdn branch January 9, 2025 18:26
@cetinbug
Copy link

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