Support read-only default mounts #3061
Conversation
| [InlineData(Chunker.MinPushBufferSize + 0, "3C7D506720601D668D8AD9DE23112591876F3021D411D51F377BF6CF7B2A453C")] | ||
| [InlineData(Chunker.MinPushBufferSize + 1, "39FB7E365F622543D01DE46F1BE4F51E870E9CDF4C93A633BD29EE4A24BEDBB0")] | ||
| [InlineData(2 * Chunker.MinPushBufferSize - 1, "63B06CEB8ECAA6747F974450446E5072A48E3F26B4AE0192FEC41DDF61B83364")] | ||
| [InlineData(2 * Chunker.MinPushBufferSize + 0, "27032B90442309EE9C4098F64AECC9BACD9B481C7A969EECFE2C56D2BDD7CA2B")] |
There was a problem hiding this comment.
Can you elaborate on this removal?
There was a problem hiding this comment.
I mentioned it in the description. MinPushBufferSize was removed as a constant in the BuildXL package.
There was a problem hiding this comment.
I wouldn't expect us to have any negative consequences with that but it's good to be cautious of
| @@ -4,9 +4,10 @@ | |||
| <!--To inherit the global NuGet package sources remove the <clear/> line below --> | |||
| <clear /> | |||
| <add key="restsdk" value="https://pkgs.dev.azure.com/mseng/PipelineTools/_packaging/nugetvssprivate/nuget/v3/index.json" /> | |||
| <add key="BuildXL.Selfhost" value="https://pkgs.dev.azure.com/ms/BuildXL/_packaging/BuildXL.Selfhost/nuget/v3/index.json" /> | |||
There was a problem hiding this comment.
Was this the special library that needed to be added? Can you elaborate on why?
There was a problem hiding this comment.
vss-api-netcore (the API contracts for ADO) depends on something called BuildXL. BuildXL depends on RocksDbSharp. I published a new version of BuildXL because vss-api-netcore depended on it. However, that new version of BuildXL depended on a new version of RocksDbSharp that was not available in our existing set of NuGet feeds. This new feed allows us to get the newest versions of these packages.
| @@ -10,7 +10,7 @@ | |||
| <OSPlatform>OS_UNKNOWN</OSPlatform> | |||
| <OSArchitecture>ARCH_UNKNOWN</OSArchitecture> | |||
| <DebugConstant></DebugConstant> | |||
| <VssApiVersion>0.5.151-private</VssApiVersion> | |||
| <VssApiVersion>0.5.153-private</VssApiVersion> | |||
There was a problem hiding this comment.
Guessing this was the API change to include getting the isreadyonly vars?
There was a problem hiding this comment.
Yes. This is the API update the makes the change to the container contract.
src/Agent.Sdk/ContainerInfo.cs
Outdated
| @@ -56,6 +57,7 @@ public ContainerInfo(Pipelines.ContainerResource container, Boolean isJobContain | |||
| this.MapDockerSocket = container.Properties.Get<bool>("mapDockerSocket", !PlatformUtil.RunningOnWindows); | |||
| this._imageOS = PlatformUtil.HostOS; | |||
| _pathMappings = new Dictionary<string, string>( PlatformUtil.RunningOnWindows ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); | |||
| this._readOnlyVolumes = container.ReadOnlyMounts != null ? new List<string>(container.ReadOnlyMounts) : null; | |||
There was a problem hiding this comment.
I'd consider
this._readOnlyVolumes = container.ReadOnlyMounts != null
? new List<string>(container.ReadOnlyMounts)
: new List<string>();
To make the isReadOnlyVolume simpler
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.9.8" Condition="$(CodeAnalysis)=='true'" /> | |||
| <PackageReference Include="Microsoft.Win32.Registry" Version="4.4.0" /> | |||
| <PackageReference Include="Microsoft.Win32.Registry" Version="4.7.0" /> | |||
There was a problem hiding this comment.
Just curious why we had to upgrade this. The functionality wasn't available in the previous version?
There was a problem hiding this comment.
Errors were being thrown in the build process because the updated version of BuildXL depended on higher versions of these packages.
|
@alex-peck the changes LGTM. How hard would it be to add a few unit tests? |
There are no unit tests currently for the container code. There is some more work to be done to be able to test this section. I will track that as a separate feature. |
|
@alex-peck Do we have Build Canary tests that can cover these changes? |
This would be a good idea. We'd have to do it once the change is deployed though, otherwise the canary tests would fail. What do you think @alex-peck? |
Sounds like a good idea |
This fixes the release build following the latest update of vss-api-netcore in #3061. Also updates the release pipeline to support a "buildStageOnly" flag that makes it easier to test such changes in the future without having to temporarily comment out parts of the build.
This fixes the release build following the latest update of vss-api-netcore in #3061. Also updates the release pipeline to support a "buildStageOnly" flag that makes it easier to test such changes in the future without having to temporarily comment out parts of the build. Co-authored-by: maroghel <maroghel@microsoft.com>
Allow default container mounts to be set to read-only.
Changes made to NuGet packages are a result of bumping vss-api-netcore. Tests removed from ChunkerTests.cs is a result of MinPushBufferSize being removed in a BuildXL update.