Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

[WIP] feat: use pull stream based mplex #1346

Closed
wants to merge 2 commits into from
Closed

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented May 9, 2018

NOTE: Im using libp2p as a git dep, so that we can test it on CI. This is temporary and should be changed once libp2p is released.

@ghost ghost assigned dryajov May 9, 2018
@ghost ghost added the status/in-progress In progress label May 9, 2018
@dryajov dryajov changed the title feat: used pull stream based mplex feat: use pull stream based mplex May 9, 2018
@dryajov dryajov mentioned this pull request May 9, 2018
2 tasks
@dryajov dryajov force-pushed the feat/pull-mplex branch from b447cf7 to ad45349 Compare May 9, 2018 18:14
@@ -122,13 +122,13 @@
"joi": "^13.2.0",
"joi-browser": "^13.0.1",
"joi-multiaddr": "^2.0.0",
"libp2p": "~0.20.4",
"libp2p": "libp2p/js-libp2p#feat/pull-mplex",
Copy link
Member

Choose a reason for hiding this comment

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

This change should not be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was specifically to test this changes on CI. Not to be merged.

Copy link
Member

Choose a reason for hiding this comment

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

@dryajov Jenkins builds pushed branches, you don't need to submit a PR also.

Is this safe to close this PR or are you going to update the branch with the new version number when published?

For the future it would be rad to get an indication that the PR is a work in progress and/or shouldn't be merged yet. I think people have been putting "WIP" in the PR title for this - maybe this should go in the guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw @diasdavid I've added the WIP tag. This should not be merged at all, or at least not until the branch I'm pointing to in the git url gets released as a version, at which point we would need to update the git url to the required version.

Jenkins builds pushed branches, you don't need to submit a PR also.

I'm not sure what you mean here?


What I'm trying to accomplish here:

The idea with this PR (and similar) is to be able to tests changes deep in the dependency tree across different modules. AFAIK, so far we don't have a clear approach to do that and our practice has been to locally link and tests everything. This IMO is incomplete, because it confines our testing to the local environment as well as hides/exposes issues specific to linking.

IMO, it would be super cool to be able to test our changes in CI before releasing a final version and not only locally. This allows us to tests the changes in an isolated environment without relying on linking, across multiple runtimes and OSs. This is what I'm trying to do here.

There are two approaches that I know of to accomplish this:

  • One is releasing a dev/beta/alpha version suffix with a build number e.g. - 0.0.1-alpha.1
    • this of course requires publishing a new version (to either NPM or a local repo) for each build every time
  • Another is using git urls that point to npm packages
    • this is the approach I took, IMO this is the least invasive and error prone approach (based on my experience)

Copy link
Member

Choose a reason for hiding this comment

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

Jenkins builds pushed branches, you don't need to submit a PR also.

I'm not sure what you mean here?

I mean, if you push your branch to this repo Jenkins will build it. I did this recently with fix/dns. I didn't open a PR because it isn't finished yet, but Jenkins built it here: https://ci.ipfs.team/blue/organizations/jenkins/IPFS%2Fjs-ipfs/detail/fix%2Fdns/2/pipeline/20/

Copy link
Member

Choose a reason for hiding this comment

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

Speaking about testing dependencies in Jenkins, I've added a feature of adding custom node modules that will override the modules defined in package.json, so you can test dependencies via CI without having to change package.json. My worry about this feature is missing to release things before merging, leading to broken modules. See https://github.com/ipfs/jenkins-libs#example-2

We can have something flag repos with a custom Jenkinsfile, as none of the JS ones should have a custom one really.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanshaw ah, thanks - didn't know that. I still think the PR doesn't hurt at all, since it gives it visibility and a place to discuss things if/when they arise while running the tests. I think the lack of the [WIP] tag is where most of the confusion came from. Apologies for that.

@victorbjelkholm ah, thats a cool feature - I'll keep it in mind.

My worry about this feature is missing to release things before merging, leading to broken modules.

I'm not sure if this increases the changes for us to releasing broken modules, maybe I'm missing something, but that should not make our review/release filters. If anything, we can add a point to our release checklist to check for this specifically?

In any case, I'm glad we're having this discussion and I'm open to suggestions for different/better approaches 👍

@dryajov dryajov changed the title feat: use pull stream based mplex [WIP] feat: use pull stream based mplex May 14, 2018
@daviddias
Copy link
Member

Work on libp2p/js-libp2p-mplex#76

@daviddias daviddias closed this Jun 5, 2018
@ghost ghost removed the status/in-progress In progress label Jun 5, 2018
@daviddias daviddias deleted the feat/pull-mplex branch June 5, 2018 10:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants