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

Admin CLI for sending announcements to all identities #1031

Closed

Conversation

ghost
Copy link

@ghost ghost commented Jan 23, 2025

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration tests.
  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.

@ghost ghost added the enhancement New feature or request label Jan 23, 2025
@ghost ghost requested a review from tnotheis as a code owner January 23, 2025 12:52
@ghost ghost enabled auto-merge (squash) January 23, 2025 12:55
@tnotheis tnotheis changed the title Abl 509 admin cli sending of announcements to all identities Admin CLI for sending announcements to all identities Jan 23, 2025
Comment on lines +24 to +29
<ItemGroup>
<None Remove="appsettings.json" />
<Content Include="appsettings.json">
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary. As far as I know, appsettings.json files are copied by default. At least that's how it works in the other projects (e.g. ConsumerApi)

Copy link
Author

Choose a reason for hiding this comment

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

I'll check that

<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>
</ItemGroup>

Copy link
Member

Choose a reason for hiding this comment

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

You should add the following lines (it's done like this in all the other application .csproj's):

    <Target Name="PreBuild" BeforeTargets="Build" Condition="$(Configuration) == Debug">
        <Delete Files="$(ProjectDir)appsettings.override.json" />
        <Copy SourceFiles="..\..\..\..\appsettings.override.json" DestinationFolder="$(ProjectDir)" UseHardlinksIfPossible="true" />
    </Target>

And I just noticed that I forgot one major thing: since so far the Admin CLI only used two parameters (DB provider and DB connection string), I didn't feel the need to add the full configuration stuff). But now that we need a lot more than that, we should add configuration as we do it in the other projects, like in the ConsumerApi. See

static void LoadConfiguration(WebApplicationBuilder webApplicationBuilder, string[] strings)
{
webApplicationBuilder.Configuration.Sources.Clear();
var env = webApplicationBuilder.Environment;
webApplicationBuilder.Configuration
.AddJsonFile("appsettings.json", optional: true, reloadOnChange: false)
.AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: false)
.AddJsonFile("appsettings.override.json", optional: true, reloadOnChange: true);
if (env.IsDevelopment())
{
var appAssembly = Assembly.Load(new AssemblyName(env.ApplicationName));
webApplicationBuilder.Configuration.AddUserSecrets(appAssembly, optional: true);
}
webApplicationBuilder.Configuration.AddEnvironmentVariables();
webApplicationBuilder.Configuration.AddCommandLine(strings);
for an example.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add that

var severity = new Option<string?>("--severity")
{
IsRequired = true,
Description = "The severity of the announcement."
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to add a list of possible values to the description of this option.

Copy link
Author

Choose a reason for hiding this comment

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

I'll add that suggestion

{
var severity = severityInput switch
{
_ when string.IsNullOrWhiteSpace(severityInput) => AnnouncementSeverity.Low,
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we agree on removing the default value?

Copy link
Author

Choose a reason for hiding this comment

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

You're right. Will be removed.


if (input == null || !input.Trim().Equals("x", StringComparison.CurrentCultureIgnoreCase)) continue;

input = null;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if I'm missing something, but to me it looks as if with this line you always return null.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah looks little wired. That term is not required and gets removed (input == null ||

Comment on lines +60 to +64
if (texts.Count == 0)
{
Console.WriteLine(@"No texts provided. Exiting...");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary to do this check. This is already done by the MediatR command validator.

Copy link
Author

Choose a reason for hiding this comment

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

I see no good reson why not do an early exit, before the MeditR validator gets called, but ok I can remove that check.

Copy link
Member

Choose a reason for hiding this comment

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

I usually don't like duplicating the validation logic. More places where you have to remember to change things if you want to.
But yeah, on the other hand I'm also a friend of early returns. Debatable. :)

Copy link
Member

Choose a reason for hiding this comment

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

Adding these settings into appsettings.json is not good. The appsettings.json file is only meant for default values that make sense in any environment. But a rabbitmq event bus for example doesn't fulfill this criteria.

Copy link
Author

Choose a reason for hiding this comment

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

I'll adopt it as it's done in AdminApi

Comment on lines +61 to +64
var configuration = new ConfigurationBuilder()
.SetBasePath(Directory.GetCurrentDirectory())
.AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
.Build();
Copy link
Member

Choose a reason for hiding this comment

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

This should be replaced with what I wrote in one of my first comments.

Copy link
Author

@ghost ghost Jan 23, 2025

Choose a reason for hiding this comment

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

Ok

Comment on lines +40 to +41
# Ensure the appsettings.json file is copied to the correct location
COPY appsettings.json /app/appsettings.json
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, and BTW it doesn't work :-(

Copy link
Member

Choose a reason for hiding this comment

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

Let's see if it works once you're done with the other configuration related changes


namespace Backbone.AdminCli.Configuration;

public class DevicesCliConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class DevicesCliConfiguration
public class DevicesConfiguration

Copy link
Author

Choose a reason for hiding this comment

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

I can rename it.

@ghost ghost closed this Jan 24, 2025
auto-merge was automatically disabled January 24, 2025 19:12

Pull request was closed

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants