-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
ci(travis): Update Travis CI configuration for new paramedic #465
Conversation
Tagging: apache/cordova#72 |
package.json
Outdated
@@ -15,10 +15,11 @@ | |||
}, | |||
"repository": { | |||
"type": "git", | |||
"url": "https://github.com/apache/cordova-plugin-inappbrowser" | |||
"url": "github:apache/cordova-plugin-inappbrowser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be the complete shorthand syntax?
"repository": "github:apache/cordova-plugin-inappbrowser"
- personally I'd drop the github:
part, too. Recent npm versions should be intelligent enough to resolve this to GitHub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wrote a script that updated the existing URL. I didn't think of shortening the entire field to a string.
Yeah we can shorten it more.
https://docs.npmjs.com/files/package.json#repository
I wonder what is the order of priority if you dropped GitHub from the string.
"repository": "npm/npm"
"repository": "github:user/repo"
"repository": "gist:11081aaa281"
"repository": "bitbucket:user/repo"
"repository": "gitlab:user/repo"
Might be bad if someone else took our exist same username and repo name for in another git service and NPM decided to use the wrong one.. Or one day they screw up the order of priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could happen, but I don't believe it's that probable ;)
Having github: doesn't hurt though.
.travis.yml
Outdated
- node /tmp/paramedic/main.js --config pr/$PLATFORM --plugin $(pwd) --shouldUseSauce | ||
--buildName travis-plugin-inappbrowser-$TRAVIS_JOB_NUMBER | ||
- if [[ "$PLATFORM" != local ]]; then cordova-paramedic --config pr/$PLATFORM --plugin $(pwd) --buildName $PARAMEDIC_BUILDNAME --shouldUseSauce; fi | ||
- if [[ "$PLATFORM" =~ local ]]; then cordova-paramedic --config pr/local/ios-10.0 --plugin $(pwd) --buildName $PARAMEDIC_BUILDNAME; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. I dislike the hardcoding of ios-10.0
down here, but like that it doesn't reroute through package.json/npm script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the --config
value is append at paramedic's conf
directory path, I was thinking what if PLATFORM
was changed to local/ios-10
instead.
Then the line will remain as
- if [[ "$PLATFORM" =~ local ]]; then cordova-paramedic --config pr/$PLATFORM --plugin $(pwd) --buildName $PARAMEDIC_BUILDNAME; fi
Though PLATFORM=local/ios-10
, "$PLATFORM" =~ local
will still match.
Only thing is I have not looked into is if this environment variable is used within paramedic code... if not and only within this yaml file, I think it will be OK to change as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
Interesting test failure on Xcode 10.2, iOS 12.2: https://travis-ci.org/apache/cordova-plugin-inappbrowser/jobs/523799304
https://app.saucelabs.com/tests/2c2ca315b8af49bd8a8e5b34409032a8 |
appium/appium#12510 and appium/appium#12518 sound related - second one has a possible workaround/solution to look into. |
@janpio thanks for the two appium tickets as references, I will look into them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn’t be sending people to dev@cordova.apache.org just discuss in the issue thread. I have to approve people to send mail there ...
The rest looks great.
@purplecabbage I can drop the email. I only added it to be consistent with other repos that already had it. I will make sure to remove it from the other repos as well when I come across them. |
I guess if it’s in all the others then it’s fine. I’m not trying to make more work for any of us. I just thought it was a new thing ..
|
Newer version of the travis.yml is no suggested for merge in #478 (but is failing for the same reasons) |
Platforms affected
none
Motivation and Context
Description
Testing
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)