Added serval release version to translation build

[mudiagaobrikisil]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.69%. Comparing base (bdf43fa) to head (7b1172a).

Files with missing lines	Patch %	Lines
src/Serval/src/Serval.ApiServer/ServalSettings.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   56.67%   56.69%   +0.01%     
==========================================
  Files         299      301       +2     
  Lines       15626    15634       +8     
  Branches     2154     2154              
==========================================
+ Hits         8856     8863       +7     
- Misses       6114     6115       +1     
  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: ***@***.***>