MVP #2
MVP #2
Conversation
src/Migrator/MigrationWorkflow.cs
Outdated
| if (assemblies.Length == 0) | ||
| { | ||
| // scan all domain | ||
| _loadedTypes.AddRange(AppDomain.CurrentDomain.GetAssemblies().SelectMany(assembly => assembly.GetTypes())); |
There was a problem hiding this comment.
This is non-deterministic and should be in an activity which returns the list.
There was a problem hiding this comment.
I don't think we can achieve non-deterministic behavior here
There was a problem hiding this comment.
we absolutely can by running this in an activity
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.SourceLink.GitHub"> |
GitVersion.yml
Outdated
| @@ -0,0 +1,6 @@ | |||
| mode: ContinuousDeployment | |||
| branches: | |||
| master: | |||
There was a problem hiding this comment.
should this be main?
| master: | |
| main: |
There was a problem hiding this comment.
Yes, actually I don't know for sure, I know that behind these names there are regex statements
as here https://gitversion.net/docs/reference/configuration#branch-configuration
And yes they changed to main apparently, but also isn't being used
| @@ -0,0 +1,6 @@ | |||
| mode: ContinuousDeployment | |||
There was a problem hiding this comment.
What is this? Haven't seen it before
There was a problem hiding this comment.
You can ignore it, as it is not really used, I copied from my solutions, what it does if you would have main branch, it would version your package/dll with -rc.X, where X is being n commits from your starting point, or last version... but since it's only tag [commit] everything is ignored
global.json
Outdated
| "sdk": { | ||
| "version": "8.0.200" | ||
| } | ||
| } |
| public interface IMigration | ||
| { | ||
| /// <summary> | ||
| /// Execute. |
There was a problem hiding this comment.
| /// Execute. | |
| /// Execute the migration. |
| /// <inheritdoc /> | ||
| public TypeJsonConverter() | ||
| { | ||
| _loadedTypes = GlobalReflector |
There was a problem hiding this comment.
I feel like Global reflector should be injectable so it's easier to test.
test/Migrator/TemporalTests.cs
Outdated
| [Fact] | ||
| public async Task TemporalWorkflowFromStartToFinish() | ||
| { | ||
| await _temporalClient |
There was a problem hiding this comment.
Tests for workflows should use the temporal testing framework stuff: https://github.com/temporalio/sdk-dotnet/blob/48fac0c15ffb2e7d5a36e348b1136283ac517897/tests/Temporalio.Tests/Worker/WorkflowWorkerTests.cs#L51
| /// <inheritdoc /> | ||
| public ValueTask ExecuteAsync(CancellationToken cancellationToken) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
It might actually be easier to have the workflow be started here vs in the caller
| /// </summary> | ||
| /// <param name="cancellationToken">The cancellation token.</param> | ||
| /// <returns>A <see cref="Task"/> representing the result of the asynchronous operation.</returns> | ||
| ValueTask ExecuteAsync(CancellationToken cancellationToken); |
There was a problem hiding this comment.
I think it makes sense here to expose mechanism to register dependencies in a container.
We should probably have a scoped container each migration.
| // Copyright (c) InfinityFlow. All Rights Reserved. | ||
| // Licensed under the Apache 2.0. See LICENSE file in the solution root for full license information. | ||
|
|
||
| [assembly: CLSCompliant(false)] |
Closes #1