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

Update Learnosity SDK to the latest version #89

Merged
merged 2 commits into from Jan 25, 2019

Conversation

shtrom
Copy link
Contributor

@shtrom shtrom commented Jul 12, 2018

This PR updates the SDK to the latest version.

Since v0.8.0, the SDK requires the use of composer to install it and dependencies, so the Makefile has been expanded to hide the added step of getting composer up and running with the least amount of fuss.

A ./get-composer.sh script is also provided to abstract the steps of downloading a local version.

@shtrom
Copy link
Contributor Author

shtrom commented Dec 13, 2018

Notes: still on hold. We cannot run the build step from Jenkins as it only does a remote merge without a local checkout of the code. Learnosity/salt-states#1457 is an alternative proposal, doing the build when the repo is updated.

@gonzalozawa
Copy link
Contributor

@shtrom this is not working for me. It looks like composer is changing the location of the autoload.php files so the SDK is failing on init
What steps are you following to upgrade the PHP SDK on the demos site?

@gonzalozawa
Copy link
Contributor

Actually, it looks like there were some conflicts during merge that are causing issues. Can you please merge develop to this branch and confirm if that's working fine for you?

@shtrom

This comment has been minimized.

@shtrom
Copy link
Contributor Author

shtrom commented Jan 23, 2019

@gonzalozawa ok, I've rebased onto develop, and bit the bullet: I just vendored the SDK and other dependencies. This is actually similar to what we had before, but it's managed in a cleaner way through composer. I guess this also makes it easier for beginners, as they no longer need to play with composer.

This PR is now safe to merge, after the usual round of testing of course 😉

@shtrom shtrom removed the on hold label Jan 23, 2019
@gonzalozawa
Copy link
Contributor

@shtrom thanks for this!
It's working for me, however, it looks like the README.md file still has the previous changes on this PR.
Correct me if I am wrong, but as far as I understand this latest changes, there should be no impact for clients using the demos site so probably no changes to README.md instructions and no need to use composer here https://github.com/Learnosity/learnosity-demos/tree/feature/sdk-composer#using-phps-native-server

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>
@shtrom
Copy link
Contributor Author

shtrom commented Jan 23, 2019

Ok, I updated the README. I kept a reference to composer for education purposes, but it's at the end now.

This is not great practice for production code, but this makes it easier to
get started.

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>
@gonzalozawa
Copy link
Contributor

@shtrom thanks for that. Looks good to me. I am happy to get this merged now.

@shtrom
Copy link
Contributor Author

shtrom commented Jan 25, 2019 via email

@gonzalozawa gonzalozawa merged commit 56571f3 into develop Jan 25, 2019
@gonzalozawa gonzalozawa deleted the feature/sdk-composer branch January 25, 2019 10:19
gonzalozawa pushed a commit that referenced this pull request Feb 14, 2019
* [FEATURE] Install Learnosity SDK using composer
* [VENDOR] Vendor learnosity-sdk-php and dependencies

This is not a great practice for production code, but this makes it easier to
get started.

Signed-off-by: Olivier Mehani <olivier.mehani@learnosity.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants