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

Set the fetch option to true in plugman #617

Merged
merged 1 commit into from Jul 8, 2018
Merged

Conversation

@dpogue
Copy link
Member

@dpogue dpogue commented Jun 15, 2018

This should resolve the issues brought up in these comments

Should go in to the next patch release

@dpogue dpogue requested review from stevengill and brodybits Jun 15, 2018
@brodybits
Copy link
Contributor

@brodybits brodybits commented Jun 15, 2018

I will take a look. I am wondering if this should target a minor release?

@dpogue
Copy link
Member Author

@dpogue dpogue commented Jun 15, 2018

It sounds like plugman is currently broken unless you run it with --fetch because all of the non-fetch code branches have already been removed from cordova-lib, so I think this is okay for a patch

Copy link
Contributor

@brodybits brodybits left a comment

Makes sense to me, given the little knowledge I have of this area. I wonder if we should mark the --fetch option as deprecated for future removal in the CLI?

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jul 4, 2018

Retriggering AppVeyor tests

@raphinesse raphinesse closed this Jul 4, 2018
@raphinesse raphinesse reopened this Jul 4, 2018
@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jul 4, 2018

LGTM. Should we merge it?

FYI: I'm currently preparing a PR that completely removes all fetch options and conditions really

@dpogue
Copy link
Member Author

@dpogue dpogue commented Jul 4, 2018

This was ideally intended for a patch release to fix plugman. I agree that we should go ahead and remove all the fetch-related options on master as part of the next major.

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jul 4, 2018

@dpogue I agree that it's good to have a minimal impact change for a potential patch release. So is there any reason not to merge this?

@dpogue
Copy link
Member Author

@dpogue dpogue commented Jul 4, 2018

I think it's good to go. Not sure if it makes sense to cherry-pick it into a patch version branch or just merge to master and then remove all the fetch options after.

@raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Jul 4, 2018

IDK 🤷‍♂️ I'd go with the latter, since it allows us to take immediate action

@dpogue
Copy link
Member Author

@dpogue dpogue commented Jul 4, 2018

👍 We can always cherry pick it from master to wherever it's needed

@raphinesse raphinesse merged commit ad534eb into apache:master Jul 8, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphinesse raphinesse mentioned this pull request Sep 11, 2018
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants