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

Ees 5660 replace versioned draft folder with static folder #5403

Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion infrastructure/templates/template.json
Original file line number Diff line number Diff line change
Expand Up @@ -1197,7 +1197,10 @@
"[concat('Microsoft.Web/sites/', variables('importerAppName'))]",
"[concat('Microsoft.Web/sites/', variables('contentAppName'))]",
"[concat('Microsoft.Web/sites/', variables('dataAppName'))]"
]
],
"publicDataFileshareMountPath": "/data/public-api-data",
"publicDataFileshareName": "[concat(parameters('subscription'), '-ees-papi-fs-data')]",
Comment on lines +1201 to +1202
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a nitpick, but why have we contracted the term 'fileshare'?

Azure refers to it as 'file share' across the board so it doesn't really make sense to contract it into one word.

If it's not too much trouble, would suggest we re-case this as two words i.e. publicDataFileShareMountPath and publicDataFileShareName

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great idea, but I'd prefer not to muddy the waters of this PR with it. Instead, I'll raise a separate PR and find all occurrences that we can change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #5427 which targets this PR.

If you're happy with that PR and this one, I'll firstly merge that one into this one and then this one into dev.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, makes sense. However, would suggest that we merge this in first and then get the other one merged in separately to keep things cleaner

"publicDataStorageAccountName": "[concat(parameters('subscription'), 'eespapisa')]"
},
"resources": [
{
Expand Down Expand Up @@ -3283,9 +3286,32 @@
"App:NotifierStorageConnectionString": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-storage-notifications')).secretUriWithVersion, ')')]",
"App:PublicStorageConnectionString": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-storage-public')).secretUriWithVersion, ')')]",
"App:PublisherStorageConnectionString": "[concat('@Microsoft.KeyVault(SecretUri=', reference(variables('ees-storage-publisher')).secretUriWithVersion, ')')]",
"DataFiles:BasePath": "[parameters('publicDataFileshareMountPath')]",
"PublicDataDbExists": "[parameters('publicDataDbExists')]"
}
},
{
"condition": "[parameters('publicDataDbExists')]",
"name": "[concat(variables('publisherAppName'), '/azurestorageaccounts')]",
"type": "Microsoft.Web/sites/config",
"location": "[resourceGroup().location]",
"apiVersion": "2019-08-01",
"dependsOn": [
"[resourceId('Microsoft.Web/sites', variables('publisherAppName'))]",
"[variables('publicDataFileshareMountPath')]",
"[variables('publicDataFileshareName')]",
"[variables('publicDataStorageAccountName')]"
],
"properties": {
"[variables('publicDataFileshareName')]": {
"type": "AzureFiles",
"accountName": "[variables('publicDataStorageAccountName')]",
"shareName": "[variables('publicDataFileshareName')]",
"mountPath": "[variables('publicDataFileshareMountPath')]",
"protocol": "Smb"
}
}
},
{
"type": "Microsoft.Storage/storageAccounts",
"name": "[variables('publisherStorageAccountName')]",
Expand Down

This file was deleted.

14 changes: 11 additions & 3 deletions src/GovUk.Education.ExploreEducationStatistics.Admin/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using GovUk.Education.ExploreEducationStatistics.Admin.Database;
using GovUk.Education.ExploreEducationStatistics.Admin.Hubs;
using GovUk.Education.ExploreEducationStatistics.Admin.Hubs.Filters;
using GovUk.Education.ExploreEducationStatistics.Admin.Migrations.Custom;
using GovUk.Education.ExploreEducationStatistics.Admin.Models;
using GovUk.Education.ExploreEducationStatistics.Admin.Options;
using GovUk.Education.ExploreEducationStatistics.Admin.Requests.Public.Data;
Expand Down Expand Up @@ -782,8 +781,11 @@ private void UpdateDatabase(IApplicationBuilder app, IWebHostEnvironment env)
{
context.Database.SetCommandTimeout(int.MaxValue);
context.Database.Migrate();
}

ApplyCustomMigrations();
if (!env.IsIntegrationTest())
{
ApplyCustomMigrations(app);
}
}

Expand All @@ -798,8 +800,14 @@ private void UpdateDatabase(IApplicationBuilder app, IWebHostEnvironment env)
}
}

private static void ApplyCustomMigrations(params ICustomMigration[] migrations)
private static void ApplyCustomMigrations(IApplicationBuilder app)
{
using var serviceScope = app.ApplicationServices
.GetRequiredService<IServiceScopeFactory>()
.CreateScope();

var migrations = serviceScope.ServiceProvider.GetServices<ICustomMigration>();

foreach (var migration in migrations)
{
migration.Apply();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,96 @@ public void ContainsAll_SourceListContainsAllValues_ReturnsTrue()
Assert.True(containsAll);
}

public class CartesianTests
{
[Fact]
public void TwoLists_Cartesian()
{
List<int> list1 = [1, 2];
List<string> list2 = ["3", "4"];

List<Tuple<int, string>> expected =
[
new Tuple<int, string>(1, "3"),
new Tuple<int, string>(1, "4"),
new Tuple<int, string>(2, "3"),
new Tuple<int, string>(2, "4")
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to specify the type - can just be new(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done along with the change of tuple types ta.

];

var actual = list1.Cartesian(list2);
Assert.Equal(expected, actual);
}

[Fact]
public void TwoLists_EmptyList()
{
List<int> list1 = [1, 2];
Assert.Empty(list1.Cartesian(new List<string>()));
}

[Fact]
public void TwoLists_NullList()
{
List<int> list1 = [1, 2];
Assert.Empty(list1.Cartesian((List<string>?) null));
}

[Fact]
public void ThreeLists_Cartesian()
{
List<int> list1 = [1, 2];
List<string> list2 = ["3", "4"];
List<char> list3 = ['5', '6'];

List<Tuple<int, string, char>> expected =
[
new Tuple<int, string, char>(1, "3", '5'),
new Tuple<int, string, char>(1, "3", '6'),
new Tuple<int, string, char>(1, "4", '5'),
new Tuple<int, string, char>(1, "4", '6'),
new Tuple<int, string, char>(2, "3", '5'),
new Tuple<int, string, char>(2, "3", '6'),
new Tuple<int, string, char>(2, "4", '5'),
new Tuple<int, string, char>(2, "4", '6'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to specify the type - can just be new(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done along with the change of tuple types ta.

];

var actual = list1.Cartesian(list2, list3);
Assert.Equal(expected, actual);
}

[Fact]
public void ThreeLists_EmptyFirstList()
{
List<int> list1 = [1, 2];
List<char> list3 = ['5', '6'];
Assert.Empty(list1.Cartesian(new List<string>(), list3));
}

[Fact]
public void ThreeLists_EmptySecondList()
{
List<int> list1 = [1, 2];
List<string> list2 = ["3", "4"];
Assert.Empty(list1.Cartesian(list2, new List<char>()));
}

[Fact]
public void ThreeLists_NullFirstList()
{
List<int> list1 = [1, 2];
List<char> list3 = ['5', '6'];
Assert.Empty(list1.Cartesian((List<string>?) null, list3));
}

[Fact]
public void ThreeLists_NullSecondList()
{
List<int> list1 = [1, 2];
List<string> list2 = ["3", "4"];
Assert.Empty(list1.Cartesian(list2, (List<char>?) null));
}
}

private static async Task<Either<Unit, int>> GetSuccessfulEither(int value)
{
await Task.Delay(5);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
namespace GovUk.Education.ExploreEducationStatistics.Common.Database;

/// <summary>
/// Custom migrations that run on deployment / startup.
/// These differ in use from EF database migrations in that they are not limited
/// to running against a single database / DbContext. These can be simple
/// standalone implementations or can use dependency injection if registered
/// with a DI container.
/// </summary>
public interface ICustomMigration
{
void Apply();
}
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public static IEnumerable<T> WhereNotNull<T>(this IEnumerable<T?> source)
}

public static IAsyncEnumerable<T> WhereNotNull<T>(this IAsyncEnumerable<T?> source)
where T: class
where T : class
{
return source.Where(item => item is not null)!;
}
Expand Down Expand Up @@ -264,7 +264,7 @@ public static bool IsSameAsIgnoringOrder<T>(this IEnumerable<T> first, IEnumerab

return !(firstNotInSecond.Any() || secondNotInFirst.Any());
}

public static Tuple<T, T> ToTuple2<T>(this IEnumerable<T> collection)
where T : class
{
Expand All @@ -275,10 +275,10 @@ public static Tuple<T, T> ToTuple2<T>(this IEnumerable<T> collection)
throw new ArgumentException(
$"Expected 2 list items when constructing a 2-tuple, but found {list.Count}");
}

return new Tuple<T, T>(list[0], list[1]);
}

public static Tuple<T, T, T> ToTuple3<T>(this IEnumerable<T> collection)
where T : class
{
Expand All @@ -289,15 +289,15 @@ public static Tuple<T, T, T> ToTuple3<T>(this IEnumerable<T> collection)
throw new ArgumentException(
$"Expected 3 list items when constructing a 3-tuple, but found {list.Count}");
}

return new Tuple<T, T, T>(list[0], list[1], list[2]);
}

public static bool ContainsAll<T>(this IEnumerable<T> source, IEnumerable<T> values)
{
return values.All(id => source.Contains(id));
}

/// <summary>
/// Order some objects, according to a string key, in natural order for humans to read.
/// </summary>
Expand All @@ -319,5 +319,30 @@ public static IOrderedEnumerable<T> NaturalThenBy<T>(
{
return source.ThenBy(keySelector, comparison.WithNaturalSort());
}

public static List<Tuple<T1, T2>> Cartesian<T1, T2>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason we're using the Tuple class over value tuples like List<(T1, T2)>?

There's a bit of a distinction between the two, but it seems like value tuples are newer and seem to be recommended for use over the Tuple class. Value tuples importantly are structs and allocate memory more efficiently.

Could switch to using value tuples?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great idea! Done.

this IEnumerable<T1> list1,
IEnumerable<T2>? list2)
{
return list2 == null
? []
: list1
.Join(list2, _ => true, _ => true, (t1, t2) => new Tuple<T1, T2>(t1, t2))
.ToList();
}

public static List<Tuple<T1, T2, T3>> Cartesian<T1, T2, T3>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above - could use value tuples instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a great idea! Done.

this IEnumerable<T1> list1,
IEnumerable<T2>? list2,
IEnumerable<T3>? list3)
{
return list2 == null || list3 == null
? []
: list1
.Join(list2, _ => true, _ => true, (t1, t2) => new Tuple<T1, T2>(t1, t2))
.Join(list3, _ => true, _ => true,
(tuple, t3) => new Tuple<T1, T2, T3>(tuple.Item1, tuple.Item2, t3))
.ToList();
}
}
}
Loading