feat: Include closed milestones in milestones stream

[TrishGillett]

⚠️ This may be a disruptive change for some users who are only interested in open milestones and are relying on the fact that this tap only pulls open milestones. These existing users would need to apply a stream map or downstream filter in order to continue excluding closed milestones. Update: we've landed on a way to implement it as optional, with the stream still extracting only open milestones by default.

Why is it worth making this potentially disruptive change?
If a user is using this tap to maintain and update a data source about milestones and their states/attributes, but each run only extracts data about open milestones, all past and current milestones will show up as open whether they are open or closed. Other fields (especially counts of open/closed issues, closed_at, updated_at) would contain potentially stale data corresponding to the last time they were observed before they were closed. By extracting data about all milestones, previously extracted milestones which are now closed can be updated to a closed state and have any other changed fields updated as well.

The implementation approach for this change follows the pattern used in streams like IssuesStream and PullRequestsStream when diverging from the default query parameters.

[edgarrmondragon]

@TrishGillett wdyt about putting behind a (new) setting to make it opt-in?

[TrishGillett]

@TrishGillett wdyt about putting behind a (new) setting to make it opt-in?

I'd be down for that! If we can make it non-disruptive for existing users that's a big win. Are you thinking of:

• An alternative stream similar to how stargazers rest and stargazers graphql coexist?
• Some kind of stream specific setting on the existing stream? I don't think I've seen examples of something like that so a hint or reference would be appreciated!
• A top level setting like start_date, auth_token, etc, but that would only change behaviour for this stream?

[edgarrmondragon]

A top level setting like start_date, auth_token, etc, but that would only change behaviour for this stream?

I think this is my preference. Not ideal that a top-level setting affects the behavior of a stream, but there's some precedence in the ecosystem so I think we can live with it.

[TrishGillett]

Not ideal that a top-level setting affects the behavior of a stream, but there's some precedence in the ecosystem so I think we can live with it.

Yeah, it's not my favourite either. I think the least-bad way to do a top-level setting might be to structure it in a way where we won't hate ourselves later if more cases like this come up in other streams and we need to add more.

(Full disclosure: I've been looking into Github's GraphQL API lately and thinking about how there may be reasons to make GraphQL versions of some streams, and I can picture wanting to add more stream options in that case since GraphQL offers a lot more flexibility. For example, a GraphQL issues stream could support an end date, and hopefully also an option to only get data about issues, instead of the default which includes data about PRs too.)

For a pattern, how would you feel about one of these:
Option 1 (It adds a little bulk to the config but the organization is a little nice IMO)

config:
  milestones_stream_params:
    state: all
  other_stream_params:
    thing: value_1
    other_thing: value_2

Option 2

config:
  milestones_stream_state_param: all
  other_stream_thing_param: value_1
  other_stream_other_thing_param: value_1

[edgarrmondragon]

Not ideal that a top-level setting affects the behavior of a stream, but there's some precedence in the ecosystem so I think we can live with it.

Yeah, it's not my favourite either. I think the least-bad way to do a top-level setting might be to structure it in a way where we won't hate ourselves later if more cases like this come up in other streams and we need to add more.

(Full disclosure: I've been looking into Github's GraphQL API lately and thinking about how there may be reasons to make GraphQL versions of some streams, and I can picture wanting to add more stream options in that case since GraphQL offers a lot more flexibility. For example, a GraphQL issues stream could support an end date, and hopefully also an option to only get data about issues, instead of the default which includes data about PRs too.)

For a pattern, how would you feel about one of these: Option 1 (It adds a little bulk to the config but the organization is a little nice IMO)

config:
  milestones_stream_params:
    state: all
  other_stream_params:
    thing: value_1
    other_thing: value_2

Option 2

config:
  milestones_stream_state_param: all
  other_stream_thing_param: value_1
  other_stream_other_thing_param: value_1

Hey @TrishGillett, sorry for the delay in reviewing. I like Option 1 😄. Wdyt of shorter names, e.g.

config:
  milestones:
    state: all
  all_streams:
    thing: value_1
    other_thing: value_2

Otherwise I think "nested" is the right approach.

[TrishGillett]

Cool! Will implement. 🙌

[TrishGillett]

Pushed an update. This can be considered fair game for review, although it shouldn't be merged just yet because I still need to do a test run and confirm it works as expected.

[TrishGillett]

Done test runs:

• ✅ When not using the new config, you still get only open milestones
• ✅ When using the new config with state = all, you get all milestones
  🎉

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[edgarrmondragon]

LGTM!

Thanks for your patience @TrishGillett 😄