Add client operations to support registering the Kubernetes agent as a worker #858
Conversation
There was a problem hiding this comment.
Tenants are also retrieved by name, id, or slug, Can you update that method (GetTenants) to use the new extension method?
There was a problem hiding this comment.
It turns out that only the non-async GetTenants currently supports slugs, and it is the async version that get actually gets used in Tentacle..
Nevertheless, I will adjust both to use the new extension method.
There was a problem hiding this comment.
Ah, that's probably my fault. Sorry and thanks! 🙏
(It sucks supporting both sync and async 👎 )
|
|
||
| internal static class AsyncRepositoryExtensions | ||
| { | ||
| public static async Task<List<TResource>> FindByNameIdOrSlugs<TResource, TRepository>(this TRepository repository, |
| if (missing.Any()) | ||
| throw new InvalidRegistrationArgumentsException(CouldNotFindByMultipleMessage("environment", missing.ToArray())); | ||
| var environmentsByNameIdOrSlug = | ||
| repository.Environments.FindByNameIdOrSlugs<EnvironmentResource, IEnvironmentRepository>( |
There was a problem hiding this comment.
seems odd that the FindByIdOrName would take a lambda for failures - that's the callers problem, not here's.
There was a problem hiding this comment.
Unfortunately this is because the caller needs to provide a custom error message that contains the type of resource it couldn't find (I think this is because it needs to be in the error logs in Tentacle.. and Client.Operations is largely concerned about Tentacle). In any case, the actual error message generator comes from a protected static method defined in RegisterMachineOperationBase, presumably specific to machine operations. Using a lambda to preserve existing error logging was the best solution I could come up with. I didn't want the generic extension method to start knowing about machine operation error messages.
|
|
||
| if (missing.Any()) | ||
| throw new InvalidRegistrationArgumentsException(CouldNotFindByMultipleMessage("environment", missing.ToArray())); | ||
| var environmentsByNameIdOrSlug = |
There was a problem hiding this comment.
I think the variable is literally just "existingEnvs" - I don't care how you found them, I can see that in the function you called.
in fact - just "envs" would be perfectly fine :)
There was a problem hiding this comment.
Yea either way is fine really 🤷 It also doesn't hurt to be explicit, especially because the chunk of code above it is also adding environments that are found by name to the resulting env list.
| @@ -12,6 +10,12 @@ public interface IRegisterWorkerOperation : IRegisterMachineOperationBase | |||
| /// <summary> | |||
| /// Gets or sets the worker pools that this machine should be added to. | |||
| /// </summary> | |||
| [Obsolete($"Use the {nameof(WorkerPools)} property as it supports worker pool names, slugs and Ids.")] | |||
There was a problem hiding this comment.
assuming this obsolete doesn't appear anywhere other than for coders?
Do we need to create a deprecation for this?
There was a problem hiding this comment.
The people using this library will get compiler warnings about its deprecation. I think this is enough because the only way this can get used is through code.
| } | ||
|
|
||
| async Task<List<WorkerPoolResource>> GetWorkerPools(IOctopusSpaceAsyncRepository repository) | ||
|
|
There was a problem hiding this comment.
remove whitespace -but probably done by pretty printer anyway.
Background
We would like to support running the Kubernetes Agent as a worker. To do this, Tentacle must have a way to call the worker registration API of Octopus Server. It communicates with Server via Octopus.Client Operations, which act as an abstraction over the API library.
Associated PRs
[sc-82021]
Result
IRegisterKubernetesWorkerOperationIRegisterKubernetesClusterOperationand rebrands it asIRegisterKubernetesDeploymentTargetOperationas we want to be clear in the distinction between the two (the functionality is still the same)