CMS 5 compatability #46
Conversation
GuySartorelli
force-pushed
the
pulls/5/cms5-compat
branch
from
12f61fc to
1f0a0ef
Compare
| "installer-name": "silverstripe-dynamodb", | ||
| "branch-alias": { | ||
| "dev-master": "4.0.x-dev" | ||
| } |
There was a problem hiding this comment.
Removed branch alias as a matter of course.
Removed installer-name because it wasn't actually working anyway and is non-standard to ss modules.
GuySartorelli
force-pushed
the
pulls/5/cms5-compat
branch
from
a46acd2 to
a01bb1e
Compare
| // cache credentials when IAM fetches the credentials from EC2 metadata service | ||
| // this will use doctrine/cache (included via composer) to do the actual caching into APCu | ||
| // http://docs.aws.amazon.com/aws-sdk-php/guide/latest/performance.html#cache-instance-profile-credentials | ||
| $dynamoOptions['credentials'] = new DoctrineCacheAdapter(new ApcuCache()); | ||
| $dynamoOptions['credentials'] = CredentialProvider::defaultProvider(); |
There was a problem hiding this comment.
doctrine/cache is deprecated - this is from their readme:
This library is deprecated and will no longer receive bug fixes from the Doctrine Project. Please use a different cache library, preferably PSR-6 or PSR-16 instead.
The default credential provider is memoised, which is a fancy way of saying it caches the credentials after finding them.
| AWS_REGION_NAME=us-east-1 | ||
| AWS_REGION_NAME=ap-southeast-2 |
There was a problem hiding this comment.
Makes sense to use the same example as above
| @@ -1,43 +1,40 @@ | |||
| { | |||
| "name": "silverstripe/dynamodb", | |||
There was a problem hiding this comment.
All lines were touched because I replaced tabs with spaces. May be easier to look through each commit to see what changed here and why.
| Environment::setEnv('AWS_DYNAMODB_SESSION_TABLE', $origValue); | ||
| } | ||
|
|
||
| public function testGetReturnsSessionWhenConfigured() |
There was a problem hiding this comment.
Added this because the other test could be satisfied by simply always returning null - which is obviously not correct.
GuySartorelli
force-pushed
the
pulls/5/cms5-compat
branch
from
a01bb1e to
3fe1a54
Compare
Also removed a bunch of unnecessary exclusions.
GuySartorelli
force-pushed
the
pulls/5/cms5-compat
branch
from
cc7a706 to
2a3c93f
Compare
| [](https://travis-ci.org/silverstripe/silverstripe-dynamodb) | ||
| [](https://github.com/silverstripe/silverstripe-dynamodb/actions/workflows/ci.yml) |
There was a problem hiding this comment.
Note that this is wrong in the preview because currently there is no github actions in this repo. It'll fix itself after this PR is merged.
|
Tagged as 5.0.0-beta1 |
Note there aren't really any useful tests in this module but that's out of scope for this PR. I've created a separate card for that concern: #47
For now, manually test this with a local dynamodb install (or docker image) and use the AWS CLI to confirm that the sessions table gets updated as expected.
Don't squash
Each commit here is its own concern (albeit related to the overall purpose of making this module CMS 5 compatible) - we should keep the commit history readable by keeping all of these commits.
CI Run
Because this is the first time github actions are being added to this module, they won't run on this PR (because they're not in the default branch yet).
See the CI run on creative-commoners.
AFTER MERGING
After you merge this, create a new
5.0branch off the5branch, and tag5.0.0-beta1!Parent issue