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

[EPIC] Monorepo for [email protected] and connectors #890

Closed
kjdelisle opened this issue Jan 22, 2018 · 18 comments
Closed

[EPIC] Monorepo for [email protected] and connectors #890

kjdelisle opened this issue Jan 22, 2018 · 18 comments

Comments

@kjdelisle
Copy link
Contributor

kjdelisle commented Jan 22, 2018

Setup a new monorepo for the juggler's packages.

When importing existing codebase(s) to the new monorepo, it's important to use lerna import (docs) to preserve git history.

We have turned this task into an epic and created few smaller and more manageable tasks (GitHub issues). What we need is:

  • create monorepo 
  • import juggler as-is using lerna import
  • import subset of connectors as-is -- taking mysql, mongodb, rest, kv-redis as a start

For each steps, we'll need to figure out how to build and test on Travis and locally during development.

@bajtos
Copy link
Member

bajtos commented Jan 23, 2018

@kjdelisle I believe we agreed to move all connectors maintained by our team to the same monorepo too, I think we should update acceptance criteria to include a list of connectors to migrate. Thoughts?

What's important, the import should preserve git history to make it easier for us to better understand why certain changes were made in the past, therefore the import should be done by running lerna import (docs). I'll capture this point in the issue description.

@bajtos
Copy link
Member

bajtos commented Jan 23, 2018

Host the individual packages that make up the juggler

We need to make this point more specific and provide a list of packages to import. Here is what comes to my mind:

  • loopback-datasource-juggler
  • loopback-filters
  • loopback-connector

Provide a new package that acts as the "unifying" portion of the monorepo (the "juggler")

I don't understand what are we trying to accomplish by this? IMO, we should keep using the existing juggler package as the main npm package provided by the new monorepo.

I think we should also change package names from loopback- prefix to @loopback scope while we are making these sweeping changes, and reset the version numbers back to 0.x to avoid the pain of running on pre-release alpha versions for months.

Another task to consider: ideally, the monorepo should be pre-configured with conventional commit linter and lerna conventional publisher, using similar setup as we have in loopback-next.

@virkt25
Copy link
Contributor

virkt25 commented Sep 19, 2018

The name of the monorepo should be decided ahead of time imo.

My vote is for juggler-next or data-juggler.

@virkt25
Copy link
Contributor

virkt25 commented Sep 19, 2018

Also for all the connectors in this epic, the acceptance criteria should also state AppVeyor setup. Alternatively we can also look at https://github.com/marketplace/azure-pipelines as a one stop shop for Linux, macOS, Windows.

@bajtos
Copy link
Member

bajtos commented Sep 20, 2018

Alternatively we can also look at https://github.com/marketplace/azure-pipelines as a one stop shop for Linux, macOS, Windows.

I'd rather leave migration to Azure Pipelines out of scope of this work. For example, it's not clear to me if Azure Pipeline reports build status back to GitHub's pull request status API (GitHub docs) and also whether the build results are public.

@virkt25
Copy link
Contributor

virkt25 commented Sep 20, 2018

I know for a fact that they report results back to GitHub. I’d imagine for OSS projects they offer public reports. The reason I think it should be part of the work for this is because the current criteria asks for a Travis / AppVeyor setup

@bajtos
Copy link
Member

bajtos commented Sep 21, 2018

I know for a fact that they report results back to GitHub. I’d imagine for OSS projects they offer public reports.

Cool!

The reason I think it should be part of the work for this is because the current criteria asks for a Travis / AppVeyor setup

My main concern is that we are no familiar with Azure Pipelines and don't know how much work it is to set it up + what issues we will discover after switching over. I don't want to couple the migration to monorepo with upgrade to Azure Pipelines. The task of migrating juggler to monorepo is already too big, I'd rather not add even more requirements to it.

In fact, I am going to promote this GH issue into an Epic and create several smaller stories.

I would like to work incrementally: start migrating to monorepo with the knowledge and toolset we already know. If we can figure out Azure Pipelines before or while migrating to monorepo, then we can apply the new approach. If don't yet, then I think it's better to follow what we already have in loopback-next, rather than delay monorepo migration until we figure out Azure Pipelines.

Unless you think we should use this new monorepo as a playground for checking how Azure Pipeline work? Wouldn't it be easier to use an existing repository, e.g. our e-commerce example app, for that exploration?

Anyhow, could you @virkt25 please create a spike story for Azure Pipelines? We can gather all requirements we have on our CI pipeline there and also plan the work on this spike independently on this juggler-related Epic.

@bajtos bajtos changed the title [EPIC][Juggler] Create monorepo for [email protected] [EPIC] Create monorepo for [email protected] Sep 21, 2018
@bajtos
Copy link
Member

bajtos commented Sep 21, 2018

In fact, I am going to promote this GH issue into an Epic and create several smaller stories.

LOL, looks like @dhmlau have already done that. Thank you, Diana 🙇

@bajtos bajtos changed the title [EPIC] Create monorepo for [email protected] [EPIC] Monorepo for [email protected] and connectors Sep 21, 2018
@bajtos
Copy link
Member

bajtos commented Sep 21, 2018

The name of the monorepo should be decided ahead of time imo.

+1

The sooner we agree on the name, the sooner we can create an empty GitHub repo and start creating juggler-related issues in that new repo (where they belong).

My vote is for juggler-next or data-juggler.

I like juggler-next, it's consistent with loopback-next.

@raymondfeng
Copy link
Contributor

Name wise, do we have plan to move @loopback/repository* into the new monorepo? If not, maybe we should call it connector-next as a lot of functions in loopback-datasource-juggler will be refactored into other modules. For example, the relations are now implemented in @loopback/repository.

@virkt25
Copy link
Contributor

virkt25 commented Sep 24, 2018

Anyhow, could you @virkt25 please create a spike story for Azure Pipelines?

Done. #1752

@bajtos
Copy link
Member

bajtos commented Sep 25, 2018

Name wise, do we have plan to move @loopback/repository* into the new monorepo? If not, maybe we should call it connector-next as a lot of functions in loopback-datasource-juggler will be refactored into other modules. For example, the relations are now implemented in @loopback/repository.

My vision is to have an ORM framework that can be used independently from loopback-next and use this new monorepo to host everything related to this ORM framework: APIs for defining models, runtime bits (dataSource/repository/PersistedModel/etc.), connectors, etc.

For example, if somebody wanted to use our ORM in a vanilla Express application, then they should find all packages in the new monorepo.

I cannot tell yet whether @loopback/repository* should be eventually moved to this new monorepo or not.

Regardless of that, having a monorepo for connectors only does not seem very useful to me. We need both the connectors and the runtime module invoking these connectors to live in the same monorepo, so that we can change both user-facing parts and all connectors at the same time (in a single pull request).

@dhmlau dhmlau mentioned this issue Oct 11, 2018
20 tasks
@dhmlau dhmlau removed the TOB label Nov 13, 2018
@cloudwheels
Copy link

cloudwheels commented Nov 22, 2018

Can I ask why/whether an issue "Import loopback-connector-cloudant using lerna import" is considered out of scope here (in view of being pushed back from TOB anyway)?

  • The current implementation is a somewhat sub-optimal, being largely implemented via inheritance from loopback-connector-couchdb2 using util.inherits:

(source)
util.inherits(Cloudant, CouchDB);

  • todo sample is based on cloudant , shopping on Mongo, which could lead to confusion over packages ie "Use loopback/loopback-connector-cloudant for this example...", "Use @loopback/loopback-connector-mongodb for this example..."
  • vendor agnosticity and open-sourcery aside, this is an IBM project and the integration with IBM cloud should be a smooth as possible, no?
  • cloudant has a very accessible free tier for deploying quickstart examples outside of local environment vs mongodb, with the benefit of having the CF platform for app deployment within a single (free tier) PaaS environement.
  • there may opportunities refactoring shared code for 'document databases'. For example reference .find() and .delete() issues on a DefaultHasManyRepository with MongoDB connector #1987 re: mongodb connector's strictObjectIDCoercion flag and my issues with cloudant document vs user specified ids in Auto generated id in cloudant is not a number - todo example not working with cloudant #2040 - cloudant connector does not implement strictObjectIDCoercion when it probably should. In view of there being common functionality such as this (mapping "id" to internal document "_id", or not) it could be good to maintain some parity between these connectors.

@bajtos
Copy link
Member

bajtos commented Nov 26, 2018

Can I ask why/whether an issue "Import loopback-connector-cloudant using lerna import" is considered out of scope here (in view of being pushed back from TOB anyway)?

To be honest, we haven't considered this yet.

Based on our latest discussion, we are keeping connectors in their own GitHub repositories for now, until we better understand the new juggler/repository design that is emerging from our work on LB4.

The current implementation is a somewhat sub-optimal, being largely implemented via inheritance from loopback-connector-couchdb2 using util.inherits.

In my mind, it does not really matter where the connector code is hosted (a monorepo or an own repo), the improvements can be made in either place.

Sure, if the base connector class and MongoDB + CouchDb + Cloudant connectors were living in the same repository, then refactoring changes would require less pull requests to open, but we were able to make larger changes across multiple repositories in the past too. The trick is to use npm link to "install" your local clone of loopback-connector into node_modules of the connector you are changing (e.g. loopback-connector-couchdb2).

todo sample is based on cloudant , shopping on Mongo, which could lead to confusion over packages ie "Use loopback/loopback-connector-cloudant for this example...", "Use @loopback/loopback-connector-mongodb for this example..."

LoopBack tries to provide database-agnostic persistence layer. Both examples should run on both Cloudant and MongoDB.

this is an IBM project and the integration with IBM cloud should be a smooth as possible, no?

Yes in theory. In practice, getting access to a Cloudant instance we could use for our CI builds is not so straightforward. Since Travis CI provides MongoDB instance out of the box and for free, it's easier for us to use MongoDB over Cloudant.

there may opportunities refactoring shared code for 'document databases'. For example reference #1987 re: mongodb connector's strictObjectIDCoercion flag and my issues with cloudant document vs user specified ids in #2040 - cloudant connector does not implement strictObjectIDCoercion when it probably should. In view of there being common functionality such as this (mapping "id" to internal document "_id", or not) it could be good to maintain some parity between these connectors.

Makes sense. Would you like to take a closer look at this and propose a pull request to implement these changes?

@stale
Copy link

stale bot commented Nov 21, 2019

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Nov 21, 2019
@stale
Copy link

stale bot commented Dec 21, 2019

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Dec 21, 2019
@achrinza achrinza removed the stale label Mar 19, 2020
@achrinza achrinza reopened this Mar 19, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Dec 25, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants