GH-37 patch release updates for 1.3.1 #38

brodycj · 2018-08-06T23:23:56Z

Changes

CB-14251 cordova-common@^2.2.5 update (1.3.x only)
Updates from master:
- coho audit-license-headers issues in cordova-fetch #35 add license header to spec/support/dummy-local-plugin/plugin.xml (copied from cordova-plugin-device)
- Fix repo url in package.json #20 Fix repo url in package.json
- CB-13503 fix trimID bug (CB-13503 fix trimID bug when using file:path/to/plugin #13)
- Enable support for git+http, along with unit test (Enabling support for git+http #12)
test updates, mostly from master:
- Add AppVeyor CI & Travis CI on Node.js 10 (1.3.x)
- eslint devDep updates (eslint devDep updates #28)
- [CB-14133 Avoid reinstalling already installed packages #24] Speed up tests (partial cherry-pick)
- Move and rename some e2e tests (CB-14133 Avoid reinstalling already installed packages #24)
- quick fix of fetchAndMatch for Node.js 4 (spec)
- Refactor fetch e2e tests (CB-14133 Avoid reinstalling already installed packages #24)
- Use fs.readJsonSync to read package.json ([CB-14140] Remove shelljs #23)
- [[CB-14140] Remove shelljs #23] shelljs -> fs-extra (spec only - partial)
- npm i --save-dev fs-extra@4 - quick fix needed for [[CB-14140] Remove shelljs #23] shelljs -> fs-extra in spec only (1.3.x)
- Simplify tempDir setup in tests ([CB-14140] Remove shelljs #23)
- Add workaround for failing tests on AppVeyor/Node10
- Increase test timeouts (Increase test timeouts to reduce spurious failures #19)
- Let jasmine handle promises
- Fix linting errors [spec] (Fix linting #15)
- Lint all *.js files in the repo (Fix linting #15)

Motivation & rationale:

repo url has been incorrect for quite a while, fix is desired for upcoming patch release
more explicit request for recent cordova-common@2.2.5 version is desired
test updates are wanted due to improved stability when testing on more Node.js versions
recent test updates not affected by recent change to cordova-ios to require Node.js >= 6 on master
updates from Enabling support for git+http #12 & CB-13503 fix trimID bug when using file:path/to/plugin #13 are desired:
- needed to pass new spec updates without modification
- already not released for almost a half year
- low risk of breaking something else
direct interaction with master branch which now targets next major release is not desired

Some updates not (yet) included from master:

Drop support for Node.js 4 (should not be done in non-major release update)
CB-14173 Fix cordova <platform|plugin> add --link (CB-14173 Fix cordova <platform|plugin> add --link #26)
CB-14133 Avoid fetching already installed packages (CB-14133 Avoid reinstalling already installed packages #24)
Isolate Q usage to prepare later removal (in index.js & spec/fetch-unit.spec.js) - Isolate Q usage to prepare later removal #17
Part of speed up tests by about 250% (CB-14133 Avoid reinstalling already installed packages #24)
Handle missing options (Handle missing options #22)
Simplify installation location retrieval (Simplify installation location retrieval #18)
Don't manually delete node_modules contents ([CB-14140] Remove shelljs #23)
[CB-14140] Remove shelljs #23 - Replace shelljs with fs-extra and which (changes to spec/fetch.spec.js & spec/helpers.js but not to spec/fetch-unit.spec.js, index.js or package.json are cherry-picked in this PR)

Testing

npm audit shows 0 vulnerabilities (npm@6.3.0)
npm outdated --depth=0 shows no red entries
coho audit-license-headers -r fetch shows no missing license headers
coho check-license -r fetch shows no incorrect licenses (manually verified that xmldom has acceptable MIT license option)
check that npm test & other tasks succeed on AppVeyor CI
check that npm test & other tasks succeed on Travis CI

when using file:path/to/plugin

erisu

All the changes looks OK to me.

erisu · 2018-08-08T13:54:37Z

index.js

@@ -154,7 +155,7 @@ function trimID (target) {
    var gitInfo = hostedGitInfo.fromUrl(target);
    if (gitInfo) {
        target = gitInfo.project;
-    } else if (isUrl(target)) {
+    } else if (isUrl(target) || isGitUrl(target)) {
        // strip away .git and everything that follows
        var strippedTarget = target.split('.git');


Though this was not introduced from any of these changes in the PR. I just happened to have noticed this when reading the diffs and wanted to point it out.

If for any reason the target was https://scm.git.foobar.com/secret/repo.git, it would match on scm and not repo. If written in SSH format git@scm.git.foobar.com:secret/repo.git, it would error out.

A real example would have been cloning from a gists.
https://gist.github.com/sample.git or git@gist.github.com:sample.git but luckily the Gist URL format is covered by the first if condition and is not a problem.

var gitInfo = hostedGitInfo.fromUrl(target); if (gitInfo) {

Anyways, just pointing out something I noticed when looking over the line. Most users use anyways GitHub, Gitlab, and BitBucket which the first condition covers. +Gist

Given that this code has entirely vanished in the master branch, I would not bother to fix this.

(Perfect for a new issue)

I should have looked at master before writing and re-writing my paragraph HA.

@erisu I think the perfect sample finally came to me after working all day on unit testing it: it would be possible for a user to do cordova plugin add https://scm.git.foobar.com/secret/repo.git#1.0.0, expecting to get 1.0.0 release tag, but end up with the default branch due to the bug you found in this PR. Considering that this change is both broken and gone in master I would say that it should either be reverted or removed by rebase. I will deal with this another day.

Fixing or reverting the change is not required. You can keep the changes in.

As @raphinesse said, and as I mentioned earlier, this bug actually existed even before the change.

I just happened to have noticed it while reading the change diff. It was not introduced in any of these changes. And being that this entire area of the code was removed from master it won't be a future issue.

Regarding the actual change itself, it might have fixed the user's use case and will provide support too additional git services, but it might be that most service's URL structure fits properly.

Anyways at this point, the changes are OK and releasable.

Sorry for the confusion. 😆

@erisu you are right that the bug did exist before the change, but I think it was only in case of URL not starting with git://. The change here will now trigger the bug in case of a URL starting with git://. While this would be an extremely rare bug I would rather not introduce another case of such a bug in a patch release if we can avoid it somehow.

@brodybits I tried running this sample git://scm.service.io/user/my-repo.git through the method and I still got back the repo name my-repo as expected.

The only case where I saw this rare issue to occur was when the URL had two or more .git.

In the following lines parts = strippedTarget[0].match(re);, it expected that index 0 of strippedTarget to contain the entire URL minus the .git.

Example: https://scm.service.io/user/my-repo

It would then grab the item after the last /.

In the rare faile case, if the URL was https://scm.git.service.io/user/my-repo.git, it would slit on all .git and in this case, index 0 becomes https://scm.

It believes the repo name is scm.

(Make sure to transform those into test cases for the test suite)

I don't get why we are still discussing this. The patch fixes some use cases but breaks no previously working ones. So it should be included.

raphinesse

The last two changes in 30d05ba were not part of the original commit, but probably part of the "Let jasmine handle promises" commit.

Other than that, this looks good.

to reduce spurious failures I just doubled timeouts until it worked on my local machine. The real problem is that we are cloning 50MB worth of git repos over the network in some tests.

Needed to support fs-extra in spec only in patch release FUTURE TBD may be removed from devDependencies if [apacheGH-23] shelljs -> fs-extra is completely ported to 1.3.x. NOT WANTED in master

partial cherry-pick from master, shelljs still in index.js & fetch-unit.spec.js FUTURE TBD: cherry-pick remaining changes & remove fs-extra from devDependencies

to avoid caching

This should not change the test coverage at all. It only factors out commonly used patterns into functions. Best viewed with --anchored=describe

This is actually part of the previous commit, but committed separately to have readable diffs.

- Reduce run time of longest test [...] - Reduce amount of git tests for double fetch tests

- eslint@4 devDep update - in-range eslint-* devDep updates

(already done on master)

(cherry-pick from apacheGH-36)

spec/support/dummy-local-plugin/plugin.xml (copied from cordova-plugin-device)

(in-range update)

brodycj · 2018-08-14T22:19:17Z

@raphinesse I just rebased with the stray changes in 30d05ba taken out, would like to merge (as a merge commit) once Travis goes green.

raphinesse · 2018-08-14T23:25:27Z

Can't seem to change my review to approval now that the branch is merged. But I approve 👍

daviesdoclc and others added 6 commits August 6, 2018 11:25

Enabling support for git+http (apacheGH-12)

65c4cb2

Adding a unit test [git+http] (apacheGH-12)

a382442

CB-13503 fix trimID bug (apacheGH-13)

8408a6b

when using file:path/to/plugin

Lint all *.js files in the repo (apacheGH-15)

d6df72d

Fix linting errors [spec] (apacheGH-15)

509f89b

Let jasmine handle promises

c3634dd

brodycj requested review from raphinesse and erisu August 7, 2018 00:10

erisu approved these changes Aug 8, 2018

View reviewed changes

raphinesse requested changes Aug 9, 2018

View reviewed changes

brodycj referenced this pull request Aug 9, 2018

Enabling support for git+http

1994245

This was referenced Aug 10, 2018

[WIP] unit testing of module ID in internal getPath() call (1.3.x) #39

Closed

Incorrect module ID in case of certain URL patterns #40

Closed

raphinesse and others added 16 commits August 14, 2018 18:01

Increase test timeouts (apacheGH-19)

2a562b7

to reduce spurious failures I just doubled timeouts until it worked on my local machine. The real problem is that we are cloning 50MB worth of git repos over the network in some tests.

Add workaround for failing tests on AppVeyor/Node10

073802c

Simplify tempDir setup in tests (apacheGH-23)

c0a7743

npm i --save-dev fs-extra@4 - quick fix (1.3.x)

47f5e37

Needed to support fs-extra in spec only in patch release FUTURE TBD may be removed from devDependencies if [apacheGH-23] shelljs -> fs-extra is completely ported to 1.3.x. NOT WANTED in master

[apacheGH-23] shelljs -> fs-extra (spec only - partial)

fc4e48d

partial cherry-pick from master, shelljs still in index.js & fetch-unit.spec.js FUTURE TBD: cherry-pick remaining changes & remove fs-extra from devDependencies

Use fs.readJsonSync to read package.json (apacheGH-23)

06c8259

to avoid caching

Refactor fetch e2e tests (apacheGH-24)

b920a36

This should not change the test coverage at all. It only factors out commonly used patterns into functions. Best viewed with --anchored=describe

quick fix of fetchAndMatch for Node.js 4 (spec)

5ea01a2

Move and rename some e2e tests (apacheGH-24)

2ee4edc

This is actually part of the previous commit, but committed separately to have readable diffs.

[apacheGH-24] Speed up tests (partial cherry-pick)

7634f5b

- Reduce run time of longest test [...] - Reduce amount of git tests for double fetch tests

eslint devDep updates (apacheGH-28)

19ca216

- eslint@4 devDep update - in-range eslint-* devDep updates

Add Node.js 10 to AppVeyor CI & Travis CI (1.3.x)

775810c

(already done on master)

apacheGH-20 Fix repo url in package.json

db0ca58

dos2unix spec/support/dummy-local-plugin/plugin.xml

32a96b0

(cherry-pick from apacheGH-36)

apacheGH-35 add license header to dummy-local-plugin

76b11d5

spec/support/dummy-local-plugin/plugin.xml (copied from cordova-plugin-device)

apacheGH-37 cordova-common@^2.2.5 update (1.3.x only)

17e21e7

(in-range update)

brodycj force-pushed the gh-37-patch-release-updates branch from 0996948 to 17e21e7 Compare August 14, 2018 22:05

brodycj merged commit 7dca991 into apache:1.3.x Aug 14, 2018

brodycj deleted the gh-37-patch-release-updates branch August 14, 2018 22:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37 patch release updates for 1.3.1 #38

GH-37 patch release updates for 1.3.1 #38

brodycj commented Aug 6, 2018 •

edited

Loading

erisu left a comment

erisu Aug 8, 2018 •

edited

Loading

raphinesse Aug 8, 2018

janpio Aug 8, 2018

erisu Aug 8, 2018

brodycj Aug 9, 2018

erisu Aug 10, 2018 •

edited

Loading

brodycj Aug 10, 2018

erisu Aug 10, 2018

janpio Aug 10, 2018

raphinesse Aug 10, 2018 •

edited

Loading

raphinesse left a comment •

edited

Loading

brodycj commented Aug 14, 2018

raphinesse commented Aug 14, 2018

GH-37 patch release updates for 1.3.1 #38

GH-37 patch release updates for 1.3.1 #38

Conversation

brodycj commented Aug 6, 2018 • edited Loading

Changes

Testing

erisu left a comment

Choose a reason for hiding this comment

erisu Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

raphinesse Aug 8, 2018

Choose a reason for hiding this comment

janpio Aug 8, 2018

Choose a reason for hiding this comment

erisu Aug 8, 2018

Choose a reason for hiding this comment

brodycj Aug 9, 2018

Choose a reason for hiding this comment

erisu Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

brodycj Aug 10, 2018

Choose a reason for hiding this comment

erisu Aug 10, 2018

Choose a reason for hiding this comment

janpio Aug 10, 2018

Choose a reason for hiding this comment

raphinesse Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

raphinesse left a comment • edited Loading

Choose a reason for hiding this comment

brodycj commented Aug 14, 2018

raphinesse commented Aug 14, 2018

brodycj commented Aug 6, 2018 •

edited

Loading

erisu Aug 8, 2018 •

edited

Loading

erisu Aug 10, 2018 •

edited

Loading

raphinesse Aug 10, 2018 •

edited

Loading

raphinesse left a comment •

edited

Loading