VAULT-14735: generate mock clients for activity log #20252
Conversation
miagilepner
requested review from
raskchanky,
mpalmi,
hghaf099,
akshya96 and
a team
There was a problem hiding this comment.
Looks good. Just one pedantic nit, but we can also just merge as-is IMO.
One thing I thought of while testing: would it make sense to create a wiki or update the README with some general guidance for using this?
That might be a good place to indicate the need for thetestonly build flag and possibly provide guidance for doing this on GoLand/VSCode. It took me a minute to recall how to do add the build flag to my workspace.
Definitely, that's a great idea. I'll add that and link it here. |
|
|
||
| mountAccessor := defaultMount | ||
| if clients.Namespace != "" { | ||
| mountAccessor = "" |
There was a problem hiding this comment.
nit: There is a lot of variable overwriting for mountAccessor. I wonder if there is a way to avoid that or make it more readable.
There was a problem hiding this comment.
reworked the code to make it (hopefully) a bit clearer
| } | ||
| addingTo := m.months[month.GetMonthsAgo()] | ||
|
|
||
| for _, clients := range month.GetAll().Clients { |
There was a problem hiding this comment.
I am a bit confused, is clients a single client? Shall we use client instead if it is indeed a single client?
There was a problem hiding this comment.
clients is a single description of how to generate some number of clients. for example:
&Client{
Count: 10,
Namespace: "root",
Mount: "kv",
}
could be the value, and it means generate 10 distinct clients with namespace "root" and mount "kv".
I can change the variable to client, if that's clearer
miagilepner
force-pushed
the
miagilepner/VAULT-14735-segment-writing-0
branch
from
acd915f to
db6a19b
Compare
| ClientID: c.Id, | ||
| NamespaceID: c.Namespace, | ||
| NonEntity: c.NonEntity, | ||
| MountAccessor: mountAccessor, |
There was a problem hiding this comment.
Maybe we can add the new ClientType field here, or do you think that's more deserving of a separate PR?
There was a problem hiding this comment.
I can add it here
| clients: &generation.Data{ | ||
| Clients: &generation.Data_All{All: &generation.Clients{Clients: []*generation.Client{{ | ||
| Namespace: namespace.RootNamespaceID, | ||
| Mount: "identity/", | ||
| }}}}, | ||
| }, |
There was a problem hiding this comment.
Do you think it makes sense to create a constructor for generation.Data for future users, since there's a lot of decoration around a small set of actual data? If this is an uncommon pattern, I'm fine with leaving it as-is. Just thinking out loud.
There was a problem hiding this comment.
there will be, that's another ticket in the epic
miagilepner
force-pushed
the
miagilepner/VAULT-14735-segment-writing-0
branch
from
1e0bc25 to
c212cc3
Compare
This is the first PR for a set of changes to support writing activity log segments with generated clients. This PR introduces some types and methods to help generate clients. Note that these methods are not called, except in tests. A later PR will call these methods from the data generation endpoint.