Added serval release version to translation build

[mudiagaobrikisil]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.69%. Comparing base (bdf43fa) to head (7dc3e3a).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   56.67%   56.69%   +0.01%     
==========================================
  Files         299      299              
  Lines       15626    15633       +7     
  Branches     2154     2155       +1     
==========================================
+ Hits         8856     8863       +7     
  Misses       6114     6114              
  Partials      656      656              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ddaspit]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mudiagaobrikisil)

---

src/Serval/src/Serval.ApiServer/ServalSettings.cs line 0 at r1 (raw file):
This looks like a duplicate class.

---

src/Serval/src/Serval.Shared/Configuration/ServalSettings.cs line 3 at r1 (raw file):

namespace Serval.Shared.Configuration;

public class ServalSettings

I don't think we need a separate class for this. We can just add the property to ApiOptions. Also, I would rename the property to ServalVersion.

---

src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 30 at r1 (raw file):

    /// </example>
    public object? Options { get; init; }
    public string? ServalReleaseVersion { get; init; }

I would name this ServalVersion.

---

src/Serval/src/Serval.Translation/Models/Build.cs line 18 at r1 (raw file):

    public DateTime? DateFinished { get; init; }
    public IReadOnlyDictionary<string, object>? Options { get; init; }
    public string? ServalReleaseVersion { get; set; }

I would just name this ServalVersion.

---

src/Serval/test/Serval.E2ETests/ServalApiTests.cs line 50 at r1 (raw file):

    [Test]
    public async Task GetServalReleaseVersion()

I don't think we need an E2E test for this. We could just add an extra check in the TranslationEngineTests.StartBuildForEngineByIdAsync integration test.

[mudiagaobrikisil]

Thank you Damien. All noted and changes will be implemented.

…

On Fri, 18 Oct 2024, 9:34 pm Damien Daspit, ***@***.***> wrote: ***@***.**** requested changes on this pull request. Reviewed 10 of 10 files at r1, all commit messages. *Reviewable <https://reviewable.io/reviews/sillsdev/serval/517>* status: all files reviewed, 5 unresolved discussions (waiting on @mudiagaobrikisil <https://github.com/mudiagaobrikisil>) ------------------------------ *src/Serval/src/Serval.ApiServer/ServalSettings.cs line 0 at r1 <https://reviewable.io/reviews/sillsdev/serval/517#-O9WA2CK0EqvfQPS-Q-S:-O9WA2CK0EqvfQPS-Q-T:bv23yei> (raw file <https://github.com/sillsdev/serval/blob/7b1172aab909608fda3dca984e1c8b59adf7fb1d/src/Serval/src/Serval.ApiServer/ServalSettings.cs#L0>):* This looks like a duplicate class. ------------------------------ *src/Serval/src/Serval.Shared/Configuration/ServalSettings.cs line 3 at r1 <https://reviewable.io/reviews/sillsdev/serval/517#-O9W8kKw7Hhfax7UJW1Q:-O9W8kKw7Hhfax7UJW1R:bocju1c> (raw file <https://github.com/sillsdev/serval/blob/7b1172aab909608fda3dca984e1c8b59adf7fb1d/src/Serval/src/Serval.Shared/Configuration/ServalSettings.cs#L3>):* namespace Serval.Shared.Configuration; public class ServalSettings I don't think we need a separate class for this. We can just add the property to ApiOptions. Also, I would rename the property to ServalVersion . ------------------------------ *src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs line 30 at r1 <https://reviewable.io/reviews/sillsdev/serval/517#-O9W9CyzBRlHNHMMMem2:-O9W9CyzBRlHNHMMMem3:bmfvbg9> (raw file <https://github.com/sillsdev/serval/blob/7b1172aab909608fda3dca984e1c8b59adf7fb1d/src/Serval/src/Serval.Translation/Contracts/TranslationBuildDto.cs#L30>):* /// </example> public object? Options { get; init; } public string? ServalReleaseVersion { get; init; } I would name this ServalVersion. ------------------------------ *src/Serval/src/Serval.Translation/Models/Build.cs line 18 at r1 <https://reviewable.io/reviews/sillsdev/serval/517#-O9W85SQELhvHoY_j2kP:-O9W85SQELhvHoY_j2kQ:b-10ytq9> (raw file <https://github.com/sillsdev/serval/blob/7b1172aab909608fda3dca984e1c8b59adf7fb1d/src/Serval/src/Serval.Translation/Models/Build.cs#L18>):* public DateTime? DateFinished { get; init; } public IReadOnlyDictionary<string, object>? Options { get; init; } public string? ServalReleaseVersion { get; set; } I would just name this ServalVersion. ------------------------------ *src/Serval/test/Serval.E2ETests/ServalApiTests.cs line 50 at r1 <https://reviewable.io/reviews/sillsdev/serval/517#-O9WADXK6ZcQ4ZlU2DZH:-O9WADXK6ZcQ4ZlU2DZI:b9ezhpk> (raw file <https://github.com/sillsdev/serval/blob/7b1172aab909608fda3dca984e1c8b59adf7fb1d/src/Serval/test/Serval.E2ETests/ServalApiTests.cs#L50>):* [Test] public async Task GetServalReleaseVersion() I don't think we need an E2E test for this. We could just add an extra check in the TranslationEngineTests.StartBuildForEngineByIdAsync integration test. — Reply to this email directly, view it on GitHub <#517 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BLIZ45MWQOAJX3U7GW2PUC3Z4FWE5AVCNFSM6AAAAABQGG3BEOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNZYHEZDMMJXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ddaspit]

Reviewed 4 of 10 files at r2, 3 of 6 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mudiagaobrikisil)

---

src/Serval/src/Serval.Translation/Models/Build.cs line 18 at r4 (raw file):

    public DateTime? DateFinished { get; init; }
    public IReadOnlyDictionary<string, object>? Options { get; init; }
    public string? DeploymentVersion { get; set; } = "Unknown";

This doesn't need to have a default value. I would just make this { get; init; }.

---

src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs line 2000 at r4 (raw file):

        private readonly IServiceScope _scope;
        private readonly MongoClient _mongoClient;
        public readonly IOptionsMonitor<ApiOptions> ApiOptions;

Is this still needed?

[ddaspit]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mudiagaobrikisil)

---

src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs line 2000 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Is this still needed?

You can remove these commented lines.

[johnml1135]

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 1306 at r6 (raw file):

    private Build Map(Engine engine, TranslationBuildConfigDto source)
    {
        string deploymentVersion = configuration.GetValue<string>("DeploymentVersion") ?? "Unknown";

I think it is a little unclear pulling the Deployment version from within the map function. I would like to see it in the main controller function - either passing it as a parameter to the map function or adding to the Build it after it has been created.

[johnml1135]

src/Serval/test/Serval.ApiServer.IntegrationTests/TranslationEngineTests.cs line 1996 at r6 (raw file):

    }

    public class TestEnvironment : DisposableBase

Does this need to be public?

[johnml1135]

Reviewed 2 of 10 files at r1, 4 of 10 files at r2, 2 of 6 files at r3, 2 of 4 files at r4, 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mudiagaobrikisil)

---

src/Serval/src/Serval.ApiServer/appsettings.json line 19 at r6 (raw file):

  },
  "ServalSettings": {
    "ReleaseVersion": "1.0.0"

This should be removed - we are using DeploymentVersion.

[ddaspit]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mudiagaobrikisil)

[ddaspit]

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mudiagaobrikisil)

---

src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 12 at r7 (raw file):

    IPretranslationService pretranslationService,
    IOptionsMonitor<ApiOptions> apiOptions,
    IConfiguration configuration,

This should be assigned to a private readonly field.

[ddaspit]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mudiagaobrikisil)

[Enkidu93]

Looks good! Congrats, Mudi! 🥳

[johnml1135]

[johnml1135]

Reviewed 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mudiagaobrikisil)