-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adding template to publish to I/O Events #41
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.
@sangeetha5491 many thanks for your contribution that's a great one!
In addition to my inline comments I also wanted to discuss the addition of a delete-events generator, but I am not so sure how to design it as delete-action is already capable of deleting the event action.
@@ -33,7 +33,8 @@ class AnalyticsGenerator extends ActionGenerator { | |||
const response = { | |||
statusCode: 200, | |||
body: collections | |||
}` | |||
}`, | |||
utilFunction: undefined |
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.
@sangeetha5491 I like the idea of adding an additional placeholder for utility functions, but 3 comments:
- not have to set it to undefined, i.e. if it's not defined just leave it out of the options, it's a bit ugly to have
utilFunction: undefined
in most templates - move the placeholder in the stub after the main function
- rename it to
inlineUtilityFunctions
(plural, multiple could be defined)
@@ -43,10 +44,10 @@ class AnalyticsGenerator extends ActionGenerator { | |||
|
|||
writing () { | |||
// this.registerTransformStream(beautify({ indent_size: 2 })) | |||
this.sourceRoot(path.join(__dirname, '../templates')) | |||
this.sourceRoot(path.join(__dirname, '../../templates')) |
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 understand that we now need to share the action templates, but we could make it more explicit that this is a shared top level template folder:
- rename the top level template folder to
common-templates
- rename the
utils.js
andutils.test.js
toutils-action.js
andutils-action.test.js
, just rename those in the common-template folder not when copying them to the generated code - (optional - nitpick) in action templates define the source root to '.' and do like
testFile: './templates/getCollections.test.js'
andsharedLibFile: '../../common-templates/utils-action.js
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.
not renaming to util-action as discussed
this.props = { | ||
description: 'This is a sample action showcasing how to create a cloud event and publish to I/O Events', | ||
// eslint-disable-next-line quotes | ||
requiredParams: `['organizationId', 'apiKey', 'providerId', 'eventCode', 'payload']`, |
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.
since #44 things have changed a bit. Now you need to specify your requiredHeaders. Also organizationId is not a default parameter anymore (not in manifest nor in .env) but a request header provided by the user of the action via the x-gw-ims-org-id
header.
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.
you can take a look at this file https://github.com/adobe/generator-aio-app/pull/44/files#diff-33c1a44418497d1f2fddbe457c86e7e3R24 in the PR where a similar change moving the default paramorgId
to the header x-gw-ims-org-id
had to be done
mockLoggerInstance.error.mockReset() | ||
}) | ||
|
||
const fakeRequestParams = { organizationId: 'fakeOrgId', apiKey: 'fakeKey', providerId: 'fakeProvider', eventCode: 'fakeEventCode', payload: {hello: 'world'}, __ow_headers: { authorization: 'Bearer fakeToken' } } |
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.
here organizationId will have to be moved to the __ow_headers according to the above comment
lib/constants.js
Outdated
@@ -1,6 +1,7 @@ | |||
module.exports = { | |||
webAssetsDirname: 'web-src', | |||
actionsDirname: 'actions', | |||
eventsDirname: 'events', |
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.
that is not needed it seems
generators/add-events/index.js
Outdated
const { eventCodes } = require('../../lib/constants') | ||
|
||
// we have one actions generator per service, an action generator could generate different types of actions | ||
const eventCodeToActionGenerator = { |
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 you need eventCodes as we do for the sdkCodes. Sdk codes are actual codes used by console for describing adobe APIs.
Here you can remove the constant and the two maps
generators/add-events/index.js
Outdated
|
||
// options are inputs from CLI or yeoman parent generator | ||
this.option('skip-prompt', { default: false }) | ||
this.option('adobe-services', { type: String, default: '' }) |
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.
you don't need the adobe-services input here
generators/add-events/index.js
Outdated
} | ||
|
||
async prompting () { | ||
const eventCodesList = [eventCodes.cloudEvents] |
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.
here you can just define a list that directly points to a choice i.e.
eventGeneratorChoices = [{ name: 'Publish Cloud Events to Adobe I/O', value: path.join(__dirname, 'cloud-events/index.js') }]
@@ -31,6 +31,7 @@ class ExcReactGenerator extends Generator { | |||
this.props.adobeServices = this.options['adobe-services'].split(',').map(x => x.trim()) | |||
this.props.projectName = this.options['project-name'] | |||
this.props.sdkCodes = sdkCodes | |||
this.props.eventCodes = eventCodes |
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 eventCodes are needed in web-assets templates
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.
@sangeetha5491 I think you can get rid of the eventCodes here
label: 'please provide your Adobe I/O Events organization id and api key', | ||
vars: [ | ||
'EVENTS_ORGANIZATION_ID', | ||
'EVENTS_API_KEY' |
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.
@sangeetha5491 there is one more change to do 😅 We've just merged a PR (#57) where all actions template use now a unique SERVICE_API_KEY env variable that is globally set. So you can remove EVENTS_API_KEY
from the dotenv list and in the actionManifestConfig
you should change apiKey: '$EVENTS_API_KEY'
for apiKey: '$SERVICE_API_KEY'
🙏
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.
done
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.
some small formatting issues, then we can merge :)
|
||
async prompting () { | ||
this.props.actionName = await this.promptForActionName( | ||
'creates messages in Cloud events format and publishes to Adobe I/O Events', |
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.
Cloud
in lower case ?
.id('randomId') | ||
return cloudevent.format() | ||
} | ||
`, |
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.
please use no tab line 36 this will create whitespaces in the generated file
} else if (published === undefined) { | ||
logger.info('Published to I/O Events but there were not interested registrations') | ||
statusCode = 204 | ||
} |
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.
trailing whitespace at the eol
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 know the formatting is annoying, eventually would be good to have an auto beautifier for the generated files)
@@ -31,6 +31,7 @@ class ExcReactGenerator extends Generator { | |||
this.props.adobeServices = this.options['adobe-services'].split(',').map(x => x.trim()) | |||
this.props.projectName = this.options['project-name'] | |||
this.props.sdkCodes = sdkCodes | |||
this.props.eventCodes = eventCodes |
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.
@sangeetha5491 I think you can get rid of the eventCodes here
@@ -62,4 +62,6 @@ async function main (params) { | |||
} | |||
} | |||
|
|||
<% if (locals.inlineUtilityFunctions) { %><%- inlineUtilityFunctions %><% } %> |
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.
could you remove the blank lines 64 and 66?
oh actually before we merge there is a test failing: https://travis-ci.com/github/adobe/generator-aio-app/jobs/340844121 |
Adding templates that enable a user to publish events to Adobe I/O
Description
New option available as part of aio app init:
Events: Subscribe to Adobe Events or create custom events
On selecting this a use can select an option to create template to: Publish Cloud Events to Adobe I/O
This will result in creation of a template that accepts params: providerId, eventCode, and payload from the user, convert it to cloud events and publish to Adobe I/O
As part of these changes i have also extracted the templates folder out of add-actions to make it commonly available to add-action and add-event
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: