Add the GroupedUpdate field to CreatePullRequest

[brrygrdn]

This is a question in PR form, apologies!

As part of the grouped update spike, we are going to add a new field that is sent when the PR is created indicating if it was generated from a "Group Rule" or not.

In future this flag is likely to come along with a small value object that has the id/name/etc of the group rule.

On my spike, I get errors from the CLI that this column is not validate as part of the CreatePullRequest struct, so the obvious answer is to add it!

The less obvious part is the sequence I should following in doing this as right now the field is only sent if:

• You are using my spike
• You have an experiment enabled

My plan is to open a PR which adds the column, sent as 'false' by default to core soon ™️. As part of that, I'll need to update the end 2 end tests for the CLI to include it, so I'll potentially need to change the CLI first, but it seems like that could be a 🐔 and 🥚 problem where tests will fail somewhere until both merge.

So my question is this: would the easiest approach be to add GroupedUpdate with omitempty, on the assumption that existing end-to-end tests will still pass until I update core, then circle back and do a subsequent update to the CLI which removes the omitempty ?

[jakecoffman]

Yep, this makes sense to do first. As you can see the smoke tests are passing here so it's good!

[landongrindheim]

This is a question in PR form

I love that you're asking in public, and I'll weigh in with my limited viewpoint 😅

In future this flag is likely to come along with a small value object that has the id/name/etc of the group rule.

I like this idea, and would personally like to see it as a starting point. I'm biased toward using a more data-rich value, like a string mapping to Grouping or GroupRule. 👍 to omitempty 😄

[brrygrdn]

I like this idea, and would personally like to see it as a starting point. I'm biased toward using a more data-rich value, like a string mapping to Grouping or GroupRule. 👍 to omitempty 😄

Apologies I merged before I saw this comment - I think that's a fair call out, but the API already has the boolean flag so this is required as a compatibility change for the existing code in API.

Since we're not persisting these flags, I'm ok with the redundancy of the flag plus the boolean and we can circle back to remove the field once we iterate further?

There are still some unknowns around group rules since they can come from the user configuration ( and therefore likely have a system-wide id ) or be defined in core itself, so we might go through a few cycles before we land on the right approach.