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

Adopt monorepo - ie move openwpm-webext-instrumentation into this repo? #300

Closed
motin opened this issue May 15, 2019 · 8 comments
Closed
Assignees

Comments

@motin
Copy link
Contributor

motin commented May 15, 2019

Opening this for discussion.

I am finding various friction points when attempting to customize OpenWPM that relates to openwpm-webext-instrumentation being in a separate repository.

The main advantage of having openwpm-webext-instrumentation (the general data gathering bits that can be used in any web extension) in a separate repo is to be able to let it's development and consumption be decoupled from the OpenWPM crawl orchestration logic.

However, the mix between monorepo and multirepo currently seems to cause a net loss in development progress, with issues and PRs being split across repos, especially for modifications that require changes in both codebases to make sense.

Also, with the decision of moving the new OpenWPM web extension from a separate repo into the main OpenWPM repo, it seems we tend towards monorepo structure also for the new webext-based OpenWPM branch.

Another (smaller) advantage of having openwpm-webext-instrumentation as a separate repository is the ability to consume it directly from github using npm without having to have an npm package published, but this can be alleviated by simply publishing an npm package.

If @englehardt and @nhnt11 are also pro going monorepo here, I can setup the PR to make that happen (landing it in OpenWPM/automation/Extension/webext-instrumentation). I will also publish the appropriate npm packages.

@englehardt
Copy link
Collaborator

The main advantage of having openwpm-webext-instrumentation (the general data gathering bits that can be used in any web extension) in a separate repo is to be able to let it's development and consumption be decoupled from the OpenWPM crawl orchestration logic.

I still think this is a good idea to decouple the instrumentation from the OpenWPM crawling code. Ideally, we'd move the tests from this repo over to the instrumentation repo to decouple the development of the instrumentation even further. However, we can still refactor the tests to remove their dependency on the automation code and keep both in the same repo, if we decide to go monorepo.

However, the mix between monorepo and multirepo currently seems to cause a net loss in development progress, with issues and PRs being split across repos, especially for modifications that require changes in both codebases to make sense.

@nhnt11 has felt a bunch of pain related to this. I'm curious to hear his thoughts.

Also, with the decision of moving the new OpenWPM web extension from a separate repo into the main OpenWPM repo, it seems we tend towards monorepo structure also for the new webext-based OpenWPM branch.

The OpenWPM extension is basically a shell around the instrumentation library that has no use outside of OpenWPM's automation. So I wouldn't consider that when deciding whether to fold in the instrumentation.

Another (smaller) advantage of having openwpm-webext-instrumentation as a separate repository is the ability to consume it directly from github using npm without having to have an npm package published, but this can be alleviated by simply publishing an npm package.

+1 to publishing on npm, irrespective of our decision here.

If @englehardt and @nhnt11 are also pro going monorepo here, I can setup the PR to make that happen (landing it in OpenWPM/automation/Extension/webext-instrumentation). I will also publish the appropriate npm packages.

Since I haven't experienced these development pain points yet myself (as I haven't needed to modify the extension), I'd like to hear from @nhnt11 first on his experience.

@englehardt
Copy link
Collaborator

The fact that you have felt the need to write mozilla/openwpm-webext-instrumentation#43 points to monorepo.

@englehardt
Copy link
Collaborator

Nihanth and I spoke about this yesterday, and we're both leaning towards consolidating the repos for the time being. Our thinking is that we still need to iterate on the tests and instrumentation, and that will be much easier to develop and review if everything is in one place. Once we have a fairly stable library with self-contained tests, we can revisit moving these back our to a separate repo.

@nhnt11
Copy link
Contributor

nhnt11 commented May 28, 2019

This is a bit convoluted, and I find myself easily getting onto a "train of overthought". It sounds complicated to merge the repos now and split them up again later, but I think that development in the immediate future will be positively impacted by a mono-repo approach. The biggest problem, I think, is that the tests are in one repo and the code is in another.

We can keep things modular and split it up properly if needed when things are more mature.

@motin
Copy link
Contributor Author

motin commented Jun 25, 2019

On our meeting last week we concluded the follow long term order of events.

  1. First combine into monorepo
  2. Split tests - instrumentation tests + platform/instrumentation tests
  3. Re-split into separate instrumentation repo (with its own tests) and platform repo

This allows us to move faster towards a stable updated framework using monorepo for the foreseeable future, while maintaining the long term goal of letting the instrumentation be an independent repository and Github project.

@motin motin changed the title Follow monorepo tendency - move openwpm-webext-instrumentation into this repo? Adopt monorepo - ie move openwpm-webext-instrumentation into this repo? Jun 25, 2019
@motin motin self-assigned this Jun 25, 2019
@motin
Copy link
Contributor Author

motin commented Jun 25, 2019

TODO for monorepo:

  • Merge openwpm-webext-instrumentation repo code (Merge openwpm-webext-instrumentation repo #328)
  • Transfer tags from openwpm-webext-instrumentation repo to openwpm repo under "instrumentation/" prefix
  • Transfer issues from openwpm-webext-instrumentation repo to openwpm repo
  • Archive the openwpm-webext-instrumentation repo
  • Refactor existing work-in-progress PRs that was targeting the openwpm-webext-instrumentation repo
  • Update the wiki to reflect the new structure

After the above, we can publish to npm (#329) and add instrumentation-specific tests (#330)

@motin
Copy link
Contributor Author

motin commented Jun 27, 2019

Progress update:

  • Merge openwpm-webext-instrumentation repo code (Merge openwpm-webext-instrumentation repo #328)
  • Transfer tags from openwpm-webext-instrumentation repo to openwpm repo under "instrumentation/" prefix
  • Transfer issues from openwpm-webext-instrumentation repo to openwpm repo
  • Archive the openwpm-webext-instrumentation repo
  • Update the wiki to reflect the new structure
  • Refactor existing work-in-progress PRs that was targeting the openwpm-webext-instrumentation repo

@motin
Copy link
Contributor Author

motin commented Jun 27, 2019

Ok done:

  • Refactor existing work-in-progress PRs that was targeting the openwpm-webext-instrumentation repo

#368, #369, #370, #371, #372

@motin motin closed this as completed Jun 27, 2019
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

No branches or pull requests

3 participants