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

Index.json in Consolidated zip stores full path, not just hash #1403

Closed
wants to merge 4 commits into from

Conversation

rain-on
Copy link
Collaborator

@rain-on rain-on commented Dec 11, 2024

At the moment the index.json in consolidated calamari contains a list of hashes (aka directories) which must be consolidated in order for a given flavour of calamari to execute.

Thus when octopus is trying to construct a calamari to run, it copies ALL files from each of the listed directories into a single zip (with pathing) - and sends that over to the target.

Up until now, that works correctly.

As part of the migration to netcore - AzureWebApp required a "shim app" to wrap the capabilities in Microsoft.Deployments.Web (which does not have a netcore counterpart) - this shim-app is a netfx app - and uses newtonsoft.

When a consolidated calamari is created - the AzureWebApp now list in its hashes:

  • Directories containing required files for the netcore capabilities
  • Directories containing required files for the shim-app.

Unfortunately, this means that Newtonsoft is pulled in 3 times:

  • 👍 Once, in root directory for netcore
  • 👍 Once, in shim-app subdirectory for netfx
  • 👎 Once again in root directory, due to nature of "copy everything in hash-dir"/.

Thus - the netcore version of newtonsoft in the root-directory is overwritten with the netfx version - meaning Calamari.AzureWebApp fails to start when it is reconstructed from a consolidated calamari.

================

The solution, at this stage, is to replace the hashes, with a full-blown file-reference (See example below).

Only real concern - depending on the platform creating the index.json - you may get different path separators :/

"Packages": { "Calamari": { "PackageId": "Calamari", "Version": "2018.4.3", "IsNupkg": true, "PlatformHashes": { "netfx": [ "00e0765eb7aceea4cb85cfeeafe702d0\\Azure.Identity.dll", "00e0765eb7aceea4cb85cfeeafe702d0\\Cloud\\Azure.Identity.dll", "01656ee57f64634592a6f674815ad8b6\\Cloud\\Microsoft.Azure.Management.Network.Fluent.dll", "01656ee57f64634592a6f674815ad8b6\\Microsoft.Azure.Management.Network.Fluent.dll", "05d1b950c470ea8b0aa357f9a59cf264\\Cloud\\System.Resources.Writer.dll", "05d1b950c470ea8b0aa357f9a59cf264\\System.Resources.Writer.dll", "06d000552ed6785988ae188fc35d1b86\\Cloud\\System.Security.Cryptography.X509Certificates.dll", "06d000552ed6785988ae188fc35d1b86\\System.Security.Cryptography.X509Certificates.dll", "081d9558bbb7adce142da153b2d5577a\\Cloud\\Newtonsoft.Json.dll", "081d9558bbb7adce142da153b2d5577a\\Newtonsoft.Json.dll",

@rain-on rain-on requested a review from APErebus December 11, 2024 01:10
@@ -56,7 +56,7 @@ Dictionary<string, string[]> GroupByPlatform(IEnumerable<SourceFile> filesForPac
.GroupBy(f => f.Platform)
.ToDictionary(
g => g.Key,
g => g.Select(f => f.Hash).OrderBy(h => h).ToArray()
g => g.Select(f => Path.Combine(f.Hash, f.FullNameInDestinationArchive.Replace('/', Path.DirectorySeparatorChar))).OrderBy(h => h).ToArray()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should hardcode to use / as it's supported on windows, but \ is not supported on unix-based systems

@IsaacCalligeros95
Copy link
Contributor

We're looking to include dotnet-script in the NetFramework builds of Calamari. I missed the use case of pre/post deploy scripts on steps like deploy a package step. I've included this change on a spike pr, at this stage I'm assuming it'll help the failures we saw if it does is there any concerns with us backporting the change? (I'll obviously wait for this to be approved and merged first)

@APErebus
Copy link
Contributor

We're looking to include dotnet-script in the NetFramework builds of Calamari. I missed the use case of pre/post deploy scripts on steps like deploy a package step. I've included this change on a spike pr, at this stage I'm assuming it'll help the failures we saw if it does is there any concerns with us backporting the change? (I'll obviously wait for this to be approved and merged first)

What version do you need to backport this into?

Copy link
Contributor

@APErebus APErebus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, will wait for more info from @IsaacCalligeros95 on if we need to backport this to an LTS calamari

@IsaacCalligeros95
Copy link
Contributor

What version do you need to backport this into?

The customer issue I'm looking at is on Server 2024.3, which is on the release/27.3.5 stream of Calamari. Server 2024.4 is on 28.2.2. If it's a pain to backport these changes + the corresponding server updates I'm happy to backport it with the other changes I'm making.

@rain-on
Copy link
Collaborator Author

rain-on commented Dec 17, 2024

It's sounding like this should actually be put onto the 27.3.5 branch - then merged forward?

@APErebus
Copy link
Contributor

It's sounding like this should actually be put onto the 27.3.5 branch - then merged forward?

That would probably be easiest yep 👍

@rain-on
Copy link
Collaborator Author

rain-on commented Dec 17, 2024

Closed in favour of #1416

@rain-on rain-on closed this Dec 17, 2024
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.

3 participants