Wire 1password SDK into Calamari #1252

rain-on · 2024-05-06T06:14:05Z

This includes the following changes:

introduces Octopus.1password.sdk as a dependency of Calamari.Testing
Updates ExternalVariables.Get to fetch a secret from 1password if no env-var is avilable
Adds "secret reference to the externalVariables documented in our ticket"

NOTE: the vault in 1password is not yet updated, thus all values will be taken from an env-var, not the vault.

akirayamamoto · 2024-05-06T06:46:02Z

source/Calamari.Testing/EnvironmentVariables.cs

@@ -117,10 +151,13 @@ class EnvironmentVariableAttribute : Attribute
    {
        public string Name { get; }
        public string LastPassName { get; }


I suggest you to delete the LastPassName, assuming you will replace that with the SecretReference.

that is a great idea - but may come in a latter PR - this PR is really dedicated go getting the "EnvironmentVariables.Get" converted to async

akirayamamoto · 2024-05-06T06:48:30Z

source/Calamari.Testing/EnvironmentVariables.cs

@@ -22,25 +29,25 @@ public enum ExternalVariable
        [EnvironmentVariable("Azure_OctopusAPITester_Certificate", "Azure - OctopusAPITester")]
        AzureSubscriptionCertificate,

-        [EnvironmentVariable("GitHub_OctopusAPITester_Username", "GitHub Test Account")]
+        [EnvironmentVariable("GitHub_OctopusAPITester_Username", "GitHub Test Account", "op://Calamari Secrets for Tests/Github Octopus Api Tester Username")]


The Octopus 1P SDK is expecting a secret reference with 3 or 4 fields. This is composed of vault, secret item name, section (optional) and field name.
Example: op://Octopus Server Secrets for Tests/Azure - OctopusAPITester/password

ButThese have been updated 🙇
But will be fully resolved in later PRs.

akirayamamoto · 2024-05-06T06:49:50Z

My only concern is the Secret References are missing the field name, otherwise looks very similar to the OctopusDeploy repository from the top of my head.

eddymoulton

No glaring issues to me, just some suggestions for you to consider.

Ping me when it's out of draft and I'll come through with the approval.

eddymoulton · 2024-05-06T23:42:03Z

source/Calamari.Testing/EnvironmentVariables.cs

+                                                                    .Select(x => x.SecretReference)
+                                                                    .Where(x => x is not null)
+                                                                    .ToArray();
+            var loggerFactory = new LoggerFactory(); //.AddSerilog(Logger);


?

Suggested change

var loggerFactory = new LoggerFactory(); //.AddSerilog(Logger);

var loggerFactory = new LoggerFactory();

eddymoulton · 2024-05-06T23:43:59Z

source/Calamari.Testing/EnvironmentVariables.cs

+        {
+            var allVariableAttributes = EnvironmentVariableAttribute.GetAll()
+                                                                    .Select(x => x.SecretReference)
+                                                                    .Where(x => x is not null)


nit and/or an impossible suggestion if this codebase doesn't support it.
WhereNotNull does nullability better than .Where(x => x is not null) for whatever reason so it's a good habit to use it everywhere.

Suggested change

.Where(x => x is not null)

.WhereNotNull()

WhereNotNull is an extension method we created - furthermore - this code has been removed.

eddymoulton · 2024-05-06T23:44:59Z

source/Calamari.Testing/EnvironmentVariables.cs

+                                                                    .Where(x => x is not null)
+                                                                    .ToArray();
+            var loggerFactory = new LoggerFactory(); //.AddSerilog(Logger);
+            var microsoftLogger = loggerFactory.CreateLogger<SecretManagerClient>();


Not really familar with how the logger should be set up, but this looks unusual at the very least.
Can I assume you've tested this and it works as expected?

eddymoulton · 2024-05-06T23:47:05Z

source/Calamari.Testing/EnvironmentVariables.cs

-            Log.Warn($"The following environment variables could not be found: " +
-                     $"\n{string.Join("\n", missingVariables.Select(var => $" - {var.Name}\t\tSource: {var.LastPassName}"))}" +
-                     $"\n\nTests that rely on these variables are likely to fail.");
+            Log.Warn($"The following environment variables could not be found: " + $"\n{string.Join("\n", missingVariables.Select(var => $" - {var.Name}\t\tSource: {var.LastPassName}"))}" + $"\n\nTests that rely on these variables are likely to fail.");


Classic auto-formatter.

I'm willing to bet there is some dotsettings configuration you could change (in the shared settings) that will keep these spread across lines.

IMO it's worth taking a peek at that, or failing that manually reverting this change to the more readable original.

rolled back - haven't touched autoformatter at this stage.

eddymoulton · 2024-05-06T23:47:40Z

source/Calamari.Testing/EnvironmentVariables.cs

            {
-                throw new Exception($"Environment Variable `{attr.Name}` could not be found. The value can be found in the password store under `{attr.LastPassName}`");
+                return valueFromEnv;
+                //throw new Exception($"Environment Variable `{attr.Name}` could not be found. The value can be found in the password store under `{attr.LastPassName}`");


Suggested change

//throw new Exception($"Environment Variable `{attr.Name}` could not be found. The value can be found in the password store under `{attr.LastPassName}`");

eddymoulton · 2024-05-06T23:50:04Z

source/Calamari.Testing/EnvironmentVariables.cs

+                }
+            }
+
+            throw new Exception($"Unable to find `{attr.Name}` as either an Environment Variable, or a SecretReference. The value can be found in the password store under `{attr.LastPassName}`");


This may be slightly misleading if SecretManageIsEnabled is false? I think there is value in composing this message to say exactly what's happening, even if it only saves a single engineer a moment of debugging pain.

matt-richardson · 2024-05-07T06:28:47Z

source/Calamari.Testing/EnvironmentVariables.cs

        [EnvironmentVariable("GitHub_RateLimitingPersonalAccessToken", "GitHub test account PAT")]
        GitHubRateLimitingPersonalAccessToken,
    }

    public static class ExternalVariables
    {
+        static readonly bool SecretManagerIsEnabled = Convert.ToBoolean(Environment.GetEnvironmentVariable("CALAMARI__Tests__SecretManagerEnabled") ?? "True");


You may want to stick with OCTOPUS__Tests__SecretManagerEnabled. Maybe a benefit in consistency?

rain-on · 2024-05-08T06:52:50Z

Overtaken by #1254, Will backport comments.

rain-on · 2024-05-10T03:13:46Z

overtaken by other PRs.

APErebus · 2024-05-22T12:19:06Z

@rain-on Can this be closed?

rain-on added 4 commits May 6, 2024 15:51

wiring in the secret client

c56ff36

plumbing seems done

086995d

update the 'GetAll' function to scan for test assemblies'

410f544

upgrade version of polly

990cbc5

akirayamamoto reviewed May 6, 2024

View reviewed changes

eddymoulton reviewed May 6, 2024

View reviewed changes

matt-richardson reviewed May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wire 1password SDK into Calamari #1252

Wire 1password SDK into Calamari #1252

rain-on commented May 6, 2024

akirayamamoto May 6, 2024

rain-on May 8, 2024 •

edited

Loading

akirayamamoto May 6, 2024

rain-on May 8, 2024 •

edited

Loading

akirayamamoto commented May 6, 2024 •

edited

Loading

eddymoulton left a comment

eddymoulton May 6, 2024

eddymoulton May 6, 2024

rain-on May 8, 2024

eddymoulton May 6, 2024

eddymoulton May 6, 2024

rain-on May 8, 2024

eddymoulton May 6, 2024

rain-on May 8, 2024

eddymoulton May 6, 2024

matt-richardson May 7, 2024

rain-on commented May 8, 2024

rain-on commented May 10, 2024

APErebus commented May 22, 2024

	var loggerFactory = new LoggerFactory(); //.AddSerilog(Logger);
	var loggerFactory = new LoggerFactory();

Wire 1password SDK into Calamari #1252

Are you sure you want to change the base?

Wire 1password SDK into Calamari #1252

Conversation

rain-on commented May 6, 2024

Choose a reason for hiding this comment

rain-on May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rain-on May 8, 2024 • edited Loading

Choose a reason for hiding this comment

akirayamamoto commented May 6, 2024 • edited Loading

eddymoulton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rain-on commented May 8, 2024

rain-on commented May 10, 2024

APErebus commented May 22, 2024

rain-on May 8, 2024 •

edited

Loading

rain-on May 8, 2024 •

edited

Loading

akirayamamoto commented May 6, 2024 •

edited

Loading