-
Notifications
You must be signed in to change notification settings - Fork 1.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
file-adapters - Removed unnecessary required fields for S3Adapter #2967
file-adapters - Removed unnecessary required fields for S3Adapter #2967
Conversation
🦋 Changeset is good to goLatest commit: be17452 We got this. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
You'll need to add a changeset describing this change too. |
Co-authored-by: Mike <[email protected]>
…hub.com/TechInSite/keystone into file-adapater-remove-s3-required-params
.changeset/long-bags-itch.md
Outdated
'@keystonejs/file-adapters': major | ||
--- | ||
|
||
Removed unnecessary required fields for S3Adapter |
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.
Moved accessKeyId
, secretAccessKey
and region
to s3Options
and renamed secretAccessKey
to s3
.
Previously:
const fileAdapter = new S3Adapter({
bucket: 'bucket-name',
accessKeyId: 'ACCESS_KEY_ID',
secretAccessKey: 'SECRET_ACCESS_KEY',
region: 'us-west-2',
});
now:
const fileAdapter = new S3Adapter({
bucket: 'bucket-name',
s3Options: {
accessKeyId: 'ACCESS_KEY_ID',
s3: 'SECRET_ACCESS_KEY',
region: 'us-west-2',
}
});
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.
A couple of things:
1/ The renaming of that property from secretAccessKey
to s3
was an error in the docs. Fix forthcoming.
2/ Not sure about the accuracy of your description. These weren't "moved" anywhere. It's just that we stopped accepting them on the constructor. It was always possible (as it is after this change) to supply these properties via the s3Options
parameter.
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 don't think we need to language lawyer the changeset too much, I think the examples do must of the heavy lifting to clarify exactly what has changed and what changes a developer would need to make in order to upgrade to the new package.
If we can make that update (or something sufficiently similar) then I'm happy to give this a ✅ and get it shipped 🚀
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.
Cool. I've made all the changes I think need to be done. Anything else specific we're waiting for?
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.
Just need to update the changeset to include the info from Mike's comment then we're good to go.
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.
Yep. I think this commit takes care of that. Have I missed something?
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.
Yep, the changeset file (.changeset/long-bags-itch.md
) needs to contain the text that Mike suggested in this comment thread (or something similar). This is the text that will get put into the CHANGELOG file, and is the thing that developers will refer to when upgrading this major package version, so it's important that it clearly explains the before and after of this version change so that they can successfully this package.
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.
.
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.
This looks good now 👍 As one last thing before I merge could I get you to update your branch to master
so that it can run against CI and once that goes green I'll merge the PR. Thanks!
This PR implements the proposed solution for issue #2965 where an error was thrown if alternative AWS SDK authentication methods were used