Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add sdk-test-data as a submodule #22

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

rasendubi
Copy link
Collaborator

I'm somewhat tired of sdk-test-data updates breaking this SDK unprompted. I also want the ability to apply test-data fixes in branches (e.g., #13 is currently blocked by Eppo-exp/sdk-test-data#49).

Plan:

  • add sdk-test-data as a submodule so that we can update it when we're ready (instead of updates being pushed on us unexpectedly). (this PR)
  • make Eppo-exp/sdk-test-data open a pull request against Eppo-exp/rust-sdk when test data change. This will allow our CI validate tests and gives us a chance to adapt our code to new tests before we accept the PR. (Will open a PR on sdk-test-data after this one is merged.)

@rasendubi rasendubi force-pushed the chore-sdk-test-data-submodule branch 2 times, most recently from c1fe321 to 26e0726 Compare July 31, 2024 15:41
@rasendubi rasendubi force-pushed the chore-sdk-test-data-submodule branch from 26e0726 to 0cd59cb Compare August 2, 2024 19:24
@aarsilv
Copy link

aarsilv commented Aug 7, 2024

I'm somewhat tired of sdk-test-data updates breaking this SDK unprompted

Agree this is annoying, however given the sdk-test-data is the source of truth regarding "correct" operation, it seems like its basically an annoying way of notifying that changes have been decided. However, one way or another, we'd want that notification.

make Eppo-exp/sdk-test-data open a pull request against Eppo-exp/rust-sdk when test data change

I'm not sure this is the plan we want. Would this then make the rust SDK different than others?

If we do want to tie the SDK to snapshots of our test data (which I don't think we do), I wonder if just specifying a version hash in the checkout step would be easier

@rasendubi
Copy link
Collaborator Author

Agree this is annoying, however given the sdk-test-data is the source of truth regarding "correct" operation, it seems like its basically an annoying way of notifying that changes have been decided. However, one way or another, we'd want that notification.

Yes. I want that notification, too—just want to make it less "annoying."

I wouldn't call annoyance as the main reason though—my main concern is that it's breaking too much. It breaks main branch, all open pull requests, and all previous commits retroactively.

Besides hindering the development flow (having to take a look why CI on my PR or local tests suddenly go bonkers), it also breaks some other less frequently used tools: git bisect doesn't work, re-triggering a CI on old commit will break it, it might potentially break a release process if timing is bad, etc. (Versioning tests along the source code is always a good idea.)

Besides that, this approach is bad at notifications, too—if an SDK doesn't have new PRs open, there's no notification.

Opening a PR is a better way to notify about changes elsewhere (e.g., dependabot is a widely known example).

Would this then make the rust SDK different than others?

Hm, yes, Rust (and Ruby 😼) would differ in a small way.

If we have a good experience here though, I believe we would want to replicate that behavior to other SDKs.

If we do want to tie the SDK to snapshots of our test data (which I don't think we do), I wonder if just specifying a version hash in the checkout step would be easier

Not sure I follow. A git submodule is just specifying a version hash in the checkout step. What do you mean?

Just to clarify my proposal: when sdk-test-data's main branch updates, that would open a PR for this repo, updating a single hash of submodule to latest value (i.e., we get the notification regardless of repo activity). If the CI passes, we merge it (or maybe have it merged automatically). If CI fails, we can fix the SDK before merging. We got the notification, main remains green, PRs are not blocked, everyone's happy 🎉

@aarsilv
Copy link

aarsilv commented Aug 7, 2024

I'm probably misunderstanding how submodules work. If basically a change in sdk-test-data results in an auto-created PR for this repository to update to use the latest test data, and leaves all other repositories unaffected, then this does sound good.

It sounds conceptually similar to my suggestion, which is to have make-test-data check out a specific version of sdk-test-data, so a PR would be needed to get more recent data. But no automatic PR as in your proposal, which I think we want.

@rasendubi
Copy link
Collaborator Author

Submodule is basically a pointer to another repository commit (url + commit hash) that git can pull for you. Seems to be exactly your proposal—just using native git tool.

Because it stores a specific hash, we need some way to update it when upstream sdk-test-data changes. Submodule doesn't do anything by itself, so my proposal is for sdk-test-data CI to open a PR against this repo with new hash.

@aarsilv
Copy link

aarsilv commented Aug 9, 2024

Ok I see, so it would be the combination of the sub module plus an action in the test data repository that automatically opens a PR? Sounds good to try this out here and if we like it, employ it in other repositories.

Copy link

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for answering my questions!

@rasendubi rasendubi merged commit e02f2f8 into main Aug 10, 2024
8 checks passed
@rasendubi rasendubi deleted the chore-sdk-test-data-submodule branch August 10, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants