-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add bynder-content-workflow to apps folder and to release-please-config.json #1860
feat: add bynder-content-workflow to apps folder and to release-please-config.json #1860
Conversation
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.
It could be helpful to update the readme with some bynder-content-workflow specific information
@@ -0,0 +1,45 @@ | |||
{ | |||
"name": "gather-content-app", |
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.
Is this the intended name of the app? Just want to make sure it's not supposed to be "bynder-content-workflow"
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.
Thanks for submitting your app! I left a few comments throughout the review, I'll summarize here:
Necessary for approval:
- Remove deploy script in package.json
- add a LICENSE file (example https://github.com/contentful/marketplace-partner-apps/blob/main/apps/transifex/LICENSE)
The rest of the comments are more suggestions that we'll leave up to your best judgement.
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.
Looks like your test config is here, though there may not be any actual tests. While this looks to be a pretty well-designed and structured app, we'd also suggest added some amount of testing to increase confidence the app is working as intended.
Co-authored-by: Mitch Goudy <mgoudy91@gmail.com>
@mgoudy91 I've made the necessary changes, and also updated the app name. Thanks for spotting! |
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.
Looks great, thanks for making those changes!
@micmcgrorty Feel free to merge this whenever you'd like. We'll go ahead and start working on some of the deployment tasks awhile, and feel free to reach out to Matt Middleton around release timing if you haven't done so already. Happy to answer any questions about the process here as well. |
Thanks for approving @mgoudy91. I was just about to merge it but it appears that I don't have the permissions, so could you please merge for me? |
My bad, will do so! |
This PR adds the Bynder Content Workflow app to the
/apps
folder and to therelease-please-config.json
file.Help documentation for the integration can be found here: https://support.bynder.com/hc/en-us/articles/17407625367954-Contentful-Integration-for-Content-Workflow