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

license show command #374

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

license show command #374

wants to merge 5 commits into from

Conversation

gprossliner
Copy link

@gprossliner gprossliner commented Feb 13, 2025

Relevant issue here #373

There are some open points for discussion:

OutputFormatter

I use the OutputFormatFeature like other commands do. This enables --json output, which looks great. But without --json it just prints license-server, which is not very informative.

I currently find no way to behave the OutputFormatter.WriteEntity in a different way, because it has like a constant format string $"{entity.Id} {dyn.Title ?? dyn.Name ?? dyn.Username ?? dyn.Expression}".

I could:

  • Check if !_output.Json and create a DTO object for output like new {Id=license.SubscriptionId} so this get printed instead.
  • Don't use _output but write directly to the console / Serilog
  • Document the behaviour and instruct the user to use --json

EDIT: I've implemented a OutputWrapperLicenseEntity for better text output.

Return code

If no licence is applied, should be an error returned or not? Currently I return 2, which should be distinct from a "general error".

Test cases

I could write Tests as there are some for the Apply command as well LicenseApplyTestsCasecs.cs. As soon as we get the Return code sorted out, I could write one for the unlicensed server. But because I won't publish the license file of the company I work for, I can't add a test showing a license.

Is there a way to get a sort of "Test Licence" that is cryptographically valid, but marked as Testing only? I don't know anything about how you issue licenses, but like one that sets the relevant fields to 0?

Serial: ...
Issued-By: Datalust Pty Ltd
Licensed-To: Testing
Subscription-Id: ...
Valid-From: Wed, 05 Feb 2025 08:23:23 GMT
Valid-To: Mon, 09 Feb 2075 07:21:27 GMT
Licensed-Users: 0
Install-Limit: 0
Nodes: 0
Clustered: True
Signature: ...
...

If such a license would be available for the public, other use-cases like testing automation in general, should benefit from this.

Please give me some feedback so I can make the final edits on this PR.
Thank you!

@gprossliner
Copy link
Author

Two EndToEnd tests fail with a message:

The command failed: The requested link `ClusterNodesResources` isn't available on entity `Seq.Api.Model.Root.RootEntity`.

Is there anything in this PR that could cause this error? Looks strange to me.

@nblumhardt
Copy link
Member

Thanks! That looks great.

Currently, SeqCli.Tests.EndToEnd will accept a license certificate on STDIN and this could be stashed somewhere/supplied to the tests. I'll dig into how we could create one along the lines you've described, but for the time being grabbing a trial from https://datalust.co/trial should be a bearable workaround.

RE the output wrapper entity, this looks great, but because not all licenses carry subscription ids (e.g. trials, the Individual version, etc.), output won't be optimal in those cases.

I think supplying the certificate in the output wrapper's Id field would be the way to go, and the second field (title, etc.) could just be ignored. This will mean output is blank when no licenses is applied, which seems reasonable.

Sorry about the "cluster nodes" test failure. Unfortunately we've published a 2025.1 preview container, and the tests pull this in. I'll check out what's needed to get the tests running properly again and update you when I have an answer.

@nblumhardt
Copy link
Member

We have an API client update that will work its way through to seqcli in a few days: datalust/seq-api#141. I don't think it'll change anything relied on by this PR.

@gprossliner
Copy link
Author

I've made the requested edits.

  • Should I remove the Warning "No license is currently applied to the server.", because when testing I realised that - at least in the docker container - there is always a license applied, even if it's the "free Individual license". So we would not confuse the User with a warning in code that will never be displayed.
  • The "LicenceShowTestCase.cs" test was added to check for the free license. This can be changed to a Test license if one is provided.
    This test may fail if a license is provided at stdin (I've seen how this is managed in the test project with the [CliTest] attribute). I need to make some more testing on this.

@gprossliner
Copy link
Author

@nblumhardt did you already check on this?

@nblumhardt
Copy link
Member

Hi @gprossliner! Thanks for following up; we've pulled in the Seq 2025.1 API and have the tests running on dev again. I've just kicked off a rebuild of this PR, and I'll run through your changes and comments now.

{
Log.Warning("No license is currently applied to the server.");
return 2;
}
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should remove this. If there's no license applied to the server, since the output format is effectively just what's displayed in the "License text" control, returning 0 here with no output is fine. (The rest of seqcli follows a fairly similar set of conventions, e.g. seqcli config get -k ... will produce no output if a setting is blank. This makes use in shell scripts easier.)

@nblumhardt
Copy link
Member

This test may fail if a license is provided at stdin (I've seen how this is managed in the test project with the [CliTest] attribute). I need to make some more testing on this.

The test will only see the license supplied on STDIN if it takes a dependency on LicenseSetup and awaits SetupAsync(). Otherwise, it'll still run with the single-user license, AFAIK, which seems fine for this test 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants