-
Notifications
You must be signed in to change notification settings - Fork 87
Unbound variable error from the latest version of dotnet-install.sh #136
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
Comments
@bozturkMSFT |
@bozturkMSFT it would be good to understand how this bug made it through validation, as it is taking out our builds, what are your thoughts? Perhaps there's some more validation we can add. |
This unblocks builds by working around dotnet/install-scripts#136
This unblocks builds by working around dotnet/install-scripts#136
This unblocks builds by working around dotnet/install-scripts#136
/fyi @dotnet/aspnet-build |
Offline is fine but I would appreciate a root cause analysis, not of the bug itself but of the rollout containing the bug. |
This has broken our deployments and halted building images in dev - when will the fix be rolled out? Very lucky production was not due to go out today! We use roslyn in production so the sdk is neccesary. |
@bozturkMSFT don't suppose you know any ETA when this will reach |
Hi @dave-yotta, |
Hi @bozturkMSFT, I hit this problem in the middle of conversion of our repo from master to main. I have already disabled maestro subscription for our repo. Do I need to re-enable it in order to get the fixed script? I know it hasn't been published yet, I'm just asking if getting the fix depends on working maestro subscription? |
@bozturkMSFT is there a way we can pin to a specific version of dotnet-install ? |
Hi @ManickaP, |
@dave-yotta For the time being, no. you can update your pipeline to fetch the script from GitHub though. In that case, you would be using |
Why doesn't MS give us a link which is pinned to a known version, CI builds need to be repeatable and giving us a link that is effectively tied to master is almost useless. There have been many similar problems with RVM in the past whereby the linked installer breaks and breaks CI builds and it causes people to have to tie things to commits. |
Can we pin to the git ref here at least by doing |
"Not breaking things in the first place" is nice, but not always possible. Unless the team who writes and publishes this script can control when it's deployed to https://dot.net, then it shouldn't be the recommended way to install. |
* Use temporary Github URL for dotnet-install.sh Context: dotnet/install-scripts#136 Co-authored-by: Jonathan Peppers <[email protected]>
@bozturkMSFT Any update? This has begun taking down production clusters on EBS because it restarts them by building from dockerfile... Edit: Devops team are hotfixing all services to pin this to the git ref. |
No updates yet, but hopefully soon. |
Is this script going to be available after the issue has been resolved? We don't want to use it and then have services go offline because the script is no longer available? |
Yes. The URL should continue to work regardless if the script was published to https://dot.net/v1/dotnet-install.sh or not. |
I'm not sure if the commit 7a9d5dc would be tidied up by git garbage collection on github at some point - I think this only happens if it becomes unreachable from any HEAD, but I don't know enough about git to be sure on this one. |
We expect the fix to be available at https://dot.net/v1/dotnet-install.sh within the next 30 minutes. |
@dave-yotta I found I could pin an older version from before the regression by downloading from https://raw.githubusercontent.com/dotnet/install-scripts/1ebb108764c092e7a314ff3fe1388f582cbcf89a/src/dotnet-install.ps1 |
We are waiting for the final approval in the CD pipeline, so it looks like we will pass the 30 minute mark. I will update here once the publishing is complete. |
Fix is now live. It may still take a few minutes for the caches to update. Please let me know if the issue still persists. |
@AArnott I though the same in my above comment - but there's a good chance |
@dave-yotta Nah. GitHub cannot remove commits that are referenced by refs. The commit I chose is in the history of the default branch, so unless they do a force push and remove the commit I selected from history (which seems very unlikely), I'm safe. |
@AArnott true - or if it gets rebased at some point but again that'd be a forced change to a head - I don't know, we're definitely too scared to do that here based on what's already happened that shouldn't really have happened :) |
Back in dotnet/cli days, these scripts used to live in that repo and the workflow used to be: make a PR (don't directly push to master no matter how urgent it is..) -> all CI legs get green -> PR gets reviewed/approved/merged -> changes get deployed to staging -> folks used to run smoke test -> deploy to production (dot.net). This workflow ensured that the commonly used parts are always in working state. Note, these public-facing scripts are used beyond github/dotnet and github/microsoft orgs; they are used in the wild by other CI providers and in miscellaneous dev workflows; so extra precautions are necessary. During the downtime of merely few hours, thousands of build failed in many CI systems. |
@am11 The process is still the same as before. In fact, the tests from the the original location of the scripts were also brought into this repo together with the scripts. Although we added some new tests as we made improvements, there seems to be some areas not yet covered by the tests. We are working on this and the pipelines will be updated in the coming days. |
This is good - although I don't recall seeing any GitHub checks linked on the PR that fixed this issue? That would add some reassurance at least (or maybe I am misremembering EDIT: They are just not displayed inline with the PR like I'm used to, they are on the checks tab :) ). I would add - this could have been picked up by simply running the installer once (on a debian environment I guess). Such tests would be extremely simple to add, and although not functional/unit tests, do directly test the use-cases we have. For example:
This is all that would have been needed to repro the problem we got here (obviously not the |
This unblocks builds by working around dotnet/install-scripts#136
The script exits with error due to an unbound variable. The code there seems to be assuming that the variable is set but that's only true if the download failed as of this commit which unsets the variables explicitly when starting the download.
On MacOS:
On Linux:
The text was updated successfully, but these errors were encountered: