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

Improved pipeline handling #296

Merged
merged 7 commits into from Jan 13, 2020
Merged

Conversation

@markvp
Copy link
Contributor

markvp commented Jan 10, 2020

Use name for function configurations rather than type / field
Use before/after for pipeline mapping templates rather than request / response
Improve documentation of pipeline mapping templates and function configurations

markvp added 3 commits Jan 10, 2020
Use name for function configurations rather than type / field
Use before/after for pipeline mapping templates rather than request / response
Improve documentation of pipeline mapping templates and function configurations
Fix typos and linting
Copy link
Collaborator

bboure left a comment

Thank you @markvp for your contribution.
Your changes look good, however we need to ensure not introducing breaking changes.
Please see my comments.

);
const respTemplPath = path.join(
functionConfigLocation,
tpl.response || `${tpl.type}.${tpl.field}.response.vtl`,
tpl.response || `${tpl.name}.response.vtl`,

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp I understand you point.
Thinking about this now, I guess that the previous implementation did not really make sense
However, this should still be considered a breaking change.
Can we add backward compatibility?

This comment has been minimized.

Copy link
@markvp

markvp Jan 10, 2020

Author Contributor

Given that functions don't have a type or a field, I'm not sure how anyone could have used this functionality in the past? I certainly couldn't get it to work.
Given that, I don't really see this is a breaking change as much as it is a fix of unusable code (no offence!)

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp After giving it a second thought and having a deeper analysis on this, I think you are right.
I got that those fields are indeed not present in pipeline definitions, but I thought that someone might have used them anyway (as a hack) in order to auto-generate the filename. But I realize that it would make no sense, since you could just place the filename directly instead.

I'll accept this change 👍

const reqSuffix = tpl.kind === 'PIPELINE' ? 'before' : 'request';
const respSuffix = tpl.kind === 'PIPELINE' ? 'after' : 'response';
const reqTemplPath = path.join(config.mappingTemplatesLocation, tpl.request || `${tpl.type}.${tpl.field}.${reqSuffix}.vtl`);
const respTemplPath = path.join(config.mappingTemplatesLocation, tpl.response || `${tpl.type}.${tpl.field}.${respSuffix}.vtl`);

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp This however is a breaking change.
People probably already rely on the fact that their before/after mapping templates are called request/response.

I see no problem in adding support for .before/.after suffixes but we must keep backward compatibility with request/response.

This comment has been minimized.

Copy link
@markvp

markvp Jan 10, 2020

Author Contributor

Hmm. I understand the desire for backwards compatibility. Not sure the best way to implement that. Perhaps checking for file existence first?

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp 🤔 Well, in CloudFormations (and in this plugin), even if in the doc they refer to it as before and after, the fields that hold these template paths are still called request and response.
So I think the right thing to do is to avoid complexity (and confusion) and stick to that.

This comment has been minimized.

Copy link
@markvp

markvp Jan 12, 2020

Author Contributor

ok. removed before/after update for now

README.md Outdated
@@ -123,10 +123,12 @@ custom:
mappingTemplatesLocation: # defaults to mapping-templates
mappingTemplates:
- dataSource: # data source name
type: # type name in schema (e.g. Query, Mutation, Subscription)
type: # type name in schema (e.g. Query, Mutation, Subscription, PIPELINE)

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp type does not take PIPELINE as a value, it's under kind.

This comment has been minimized.

Copy link
@markvp

markvp Jan 10, 2020

Author Contributor

oops. fixed.

);
const respTemplPath = path.join(
functionConfigLocation,
tpl.response || `${tpl.type}.${tpl.field}.response.vtl`,
tpl.response || `${tpl.name}.response.vtl`,

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp After giving it a second thought and having a deeper analysis on this, I think you are right.
I got that those fields are indeed not present in pipeline definitions, but I thought that someone might have used them anyway (as a hack) in order to auto-generate the filename. But I realize that it would make no sense, since you could just place the filename directly instead.

I'll accept this change 👍

README.md Outdated
request: # request mapping template name
response: # response mapping template name
# pipeline resolvers use kind: PIPELINE, otherwise kind is not required
# kind: pipeline

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp not required when UNIT, but still it would be accepted. I guess it does not hurt to specify it.
Also, is ahs to be PIPELINE or UNIT (uppercase)

otherwise, it will fail

This comment has been minimized.

Copy link
@markvp

markvp Jan 12, 2020

Author Contributor

fixed

const reqSuffix = tpl.kind === 'PIPELINE' ? 'before' : 'request';
const respSuffix = tpl.kind === 'PIPELINE' ? 'after' : 'response';
const reqTemplPath = path.join(config.mappingTemplatesLocation, tpl.request || `${tpl.type}.${tpl.field}.${reqSuffix}.vtl`);
const respTemplPath = path.join(config.mappingTemplatesLocation, tpl.response || `${tpl.type}.${tpl.field}.${respSuffix}.vtl`);

This comment has been minimized.

Copy link
@bboure

bboure Jan 10, 2020

Collaborator

@markvp 🤔 Well, in CloudFormations (and in this plugin), even if in the doc they refer to it as before and after, the fields that hold these template paths are still called request and response.
So I think the right thing to do is to avoid complexity (and confusion) and stick to that.

markvp added 3 commits Jan 12, 2020
reference default UNIT value for kind
Fix resolver default naming
@bboure

This comment has been minimized.

Copy link
Collaborator

bboure commented Jan 13, 2020

Thank you @markvp LGTM

@bboure bboure merged commit 67fb9d2 into sid88in:master Jan 13, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.