Azure / Azure/azure-sdk-tools

[Asset-Sync] Pave pathway for external contributions to make it to `assets`

Open
#6,593 2 comments 0 reactions 1 assignee Claimed by @scbedd View on GitHub
Asset-Sync
Dominant language
C#
Stars
135
Forks
260
Avg merge
3d 1h
Merged PRs (30d)
143

Description

Getting an issue out here to track this improvement after speaking to @christothes and @weshaggard. Context first:

- Currently, one needs `write` access to the `azure-sdk-assets` repository to push new recordings.
- PRs with new/adjusted tests must have recordings updated.
- External contributors don't (and won't ever) have direct write access to the `azure-sdk-assets` repository.

What do we need? An easy way for external contributors to update these recordings.

@christothes reached out with a suggested pattern:

- Running test-proxy for recordings always uses a fork of the assets repo
- _Currently we use the default `git` credentials, and one cannot create a fork directly from the git CLI.
- _Need to figure out how to "automatically" create a fork, given the fact that we don't actually have access to user config outside of default git creds/git committer email.
- _If we are able to figure out how to create a fork, we will know the username of the account. Given that, we can update the proxy to always use fork for push by dynamically updating the `remote url`.
- That push produces its own PR
- _Currently, data is stored in `tags` because we deliberately went to `point in time`. We will need to refactor back to `branches` and deal with any new errors that result._
- _If we refactor back go branches, `merge conflicts` will be possible / probably expected unless we keep the **usage** like we are still using tags._
- The changes push the normal way to generate the language repo PR
- _External contributor creates PR here._
- It is smart enough to link the assets PR in the changes PR
- _proxy would update the `assets.json` target repo and tag to point at the new tag in their pipeline._
- _This would give us an easy way to find the PR against the assets repo._
- The CI knows to follow the fork assets PR to resolve the recordings to pass CIs
- _If we update the assets to their fork, this would automatically happen._
- Assets PR is approved, merging the assets and updating the linked PR assets.json
- _A build from the `assets` repo will trigger and auto-commit updates to the linked PR assets.json._
- _Possibly don't have write access to the branch, so we need a fallback option to leave a `code suggestion` or a `comment`._
- CI can optionally run again against the asset repo/main
- Parent PR can now be merged

The top level statements are from Chris, any italic sub-bullets are details or gotchas. Frankly, parts of this could definitely work, but I also have serious reservations about what we'll be expecting the proxy to do given zero configuration. Right now, we are expecting that someone's `git` config is solid, but _that's it_. We don't _require_ a PAT so it would be difficult to fork the assets repo on the fly. Willing to listen to suggestions here!

I believe that having a simple job that can be run _against a PR_ to record changes just for the updated package and then (in descending order)

- Run tests of affected packages in `record` mode.
- `test-proxy push`
- Push a commit to the branch reflecting updated `assets.json` tag.
- Leave a code suggestion on the PR (requires `assets.json` to be a changed file).
- Leave a comment with the tag they should use.

This fullfills the same purpose, means that you don't need to visually review a block of new recordings for malignant code, and has the dual purpose use of benefitting the management folks. Often all they need to do after updating a package version is to _re-run recordings_. It would be pretty cool to entirely skip that in favor of a PR comment!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.