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

CI: use enterprise build #183

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Conversation

mfkl
Copy link
Contributor

@mfkl mfkl commented Dec 1, 2022

To use a consul enterprise version, 2 things are needed:

I have tested the starting the enterprise agent locally.

@mfkl mfkl force-pushed the ci/enterprise-key branch from 724b7f2 to 5c0a99a Compare December 16, 2022 07:46
@mfkl mfkl force-pushed the ci/enterprise-key branch from 5c0a99a to 1d42c4b Compare January 5, 2023 10:12
public async Task Operator_GetLicense()
{
var license = await _client.Operator.GetConsulLicense();
Skip.If(license.StatusCode == System.Net.HttpStatusCode.NotFound, "This test requires an Enterprise Consul version");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the most elegant, but it does the job. What do you guys think?

Consul Enterprise APIs, when called on the community edition server naturally return HTTP 404.

With a XUnit SkippableFact, we can implement tests for these Enterprise version-only APIs and they can be skipped by adding this 2 line check in the beginning of any of these tests when the server is not Enterprise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility is to implement custom Fact attribute, e.g.:

    public class EnterpriseOnlyFactAttribute : FactAttribute
    {
        public EnterpriseOnlyFactAttribute()
        {
            if (string.IsNullOrEmpty(Environment.GetEnvironmentVariable("RUN_CONSUL_ENTERPRISE_TESTS")))
            {
                Skip = "Ignored non-0enterprise servers";
            }
        }
    }

And the use:

[EnterpriseOnlyFactAttribute]
public async Task Operator_GetLicense()
{
    var license = await _client.Operator.GetConsulLicense();
    ...
}

It lets us to skip enterprise-only facts without making any extra http requests. 
Also, it feels more elegant in my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

@@ -84,12 +84,22 @@ jobs:
uses: actions/setup-dotnet@v3
with:
dotnet-version: 6.0.x
- name: Setup Consul Enterprise URL
Copy link
Contributor

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 use enterprise version for PR branches and for master branch. Without running the enterprise version on branches, it is going to be a huge struggle to test changes relevant for enterprise version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please remember that the secret is not passed to workflows that are triggered by a pull request from a fork

Ok, I think the github secret (e.g. enterprise license) will need some changes too :-) @jgiannuzzi

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just run the enterprise tests when the workflow is not triggered from a fork (regardless of the branch it is triggered from?).

public async Task Operator_GetLicense()
{
var license = await _client.Operator.GetConsulLicense();
Skip.If(license.StatusCode == System.Net.HttpStatusCode.NotFound, "This test requires an Enterprise Consul version");
Copy link
Contributor

Choose a reason for hiding this comment

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

Another possibility is to implement custom Fact attribute, e.g.:

    public class EnterpriseOnlyFactAttribute : FactAttribute
    {
        public EnterpriseOnlyFactAttribute()
        {
            if (string.IsNullOrEmpty(Environment.GetEnvironmentVariable("RUN_CONSUL_ENTERPRISE_TESTS")))
            {
                Skip = "Ignored non-0enterprise servers";
            }
        }
    }

And the use:

[EnterpriseOnlyFactAttribute]
public async Task Operator_GetLicense()
{
    var license = await _client.Operator.GetConsulLicense();
    ...
}

It lets us to skip enterprise-only facts without making any extra http requests. 
Also, it feels more elegant in my opinion.

@mfkl mfkl force-pushed the ci/enterprise-key branch from 17ae2fc to 0629f41 Compare February 23, 2023 09:14
@@ -229,6 +229,44 @@ public Task<WriteResult> KeyringUse(string key, WriteOptions q, CancellationToke
{
return _client.Put("/v1/operator/keyring", new KeyringRequest() { Key = key }, q).Execute(ct);
}

public Task<QueryResult<ConsulLicense>> GetConsulLicense(string datacenter = "", CancellationToken ct = default)
Copy link
Contributor

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 comment explaining that this is enterprise-only feature

}
}

public class ConsulLicense
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -84,12 +84,22 @@ jobs:
uses: actions/setup-dotnet@v3
with:
dotnet-version: 6.0.x
- name: Setup Consul Enterprise URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just run the enterprise tests when the workflow is not triggered from a fork (regardless of the branch it is triggered from?).

@marcin-krystianc
Copy link
Contributor

Taking the liberty to merge and see whether the secret is correctly configured.

@marcin-krystianc marcin-krystianc merged commit ada7230 into G-Research:master Jul 24, 2023
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