NIFI-6332 - Add Cache Control property to PutS3Object processor#4422
NIFI-6332 - Add Cache Control property to PutS3Object processor#4422kent-nguyen wants to merge 3 commits intoapache:mainfrom
Conversation
Add new property 'Cache Control' to allow user to set the cache-control http header on the S3 object. This property is not required, and has no default value. The implementation is similar to the Content-Type property, except that this property does not allow Expression Language.
turcsanyip
left a comment
turcsanyip
left a comment
There was a problem hiding this comment.
@kent-nguyen Thanks for the contribution. I manually tested it with a simple flow and it works properly. Also ran the integration test successfully.
Is there a specific reason why expression language is not supported by this new Cache Control property?
Also left an inline comment about improving the description of the property. Please check it.
| public static final PropertyDescriptor CACHE_CONTROL = new PropertyDescriptor.Builder() | ||
| .name("Cache Control") | ||
| .displayName("Cache Control") | ||
| .description("Sets the Cache-Control HTTP header. Multiple directives are comma-separated.") |
There was a problem hiding this comment.
Could you please add a similar explanation as in case of Content Type:
"Sets the Cache-Control HTTP header indicating the caching directives of the associated object." (or similar)
There was a problem hiding this comment.
Hi @turcsanyip Thank you for checking my PR. It's great to hear you ran the tests on it. I had carefully tested it on my local too.
In contrast to the Content Type property, which can get the MIME string from the flow file itself, the flow file metadata wouldn't normally be relevant for the Cache Control property. I felt this was similar in nature to properties like Storage Class and Server Side Encryption, which are on set on the processor and do not support expression language.
I have updated the description as your suggestion. Thank you again.
There was a problem hiding this comment.
@kent-nguyen The generic approach is to add EL support with Variable Registry scope (ExpressionLanguageScope.VARIABLE_REGISTRY) at least and to support Flow File attributes (ExpressionLanguageScope.FLOWFILE_ATTRIBUTES) when it is reasonable.
Storage Class property also has the option "Reference parameter..." (actually it is provided by the framework because it is a selectable property with a drop-down list, but in fact it is similar to setting Variable Registry scoped EL support from code).
As far as I see, you are not assigned the Contributor role yet. Could you please request for "Jira contributor access" on the mailing list dev@nifi.apache.org?
Then please assign the Jira ticket to yourself.
kent-nguyen
force-pushed
the
6332_cache_control_puts3
branch
from
ce7e6df to
ab454b3
Compare
|
Apologies for the force push, realise now that wasn't a good idea. Was trying to trigger CI again since it looks to have encountered a transient error, but I'll ignore that in future. |
turcsanyip
left a comment
turcsanyip
left a comment
There was a problem hiding this comment.
+1 LGTM
Merging to main.
@kent-nguyen Please reference the JIRA ticket number in the commit message next time.
I'll add it during the merge.
Add new property 'Cache Control' to allow user to set the cache-control http header on the S3 object. This property is not required, and has no default value. The implementation is similar to the Content-Type property, except that this property does not allow Expression Language. Update property description Add support EL for cache-control property This closes apache#4422. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Add new property 'Cache Control' to allow user to set the cache-control http header on the S3 object. This property is not required, and has no default value. The implementation is similar to the Content-Type property, except that this property does not allow Expression Language. Update property description Add support EL for cache-control property This closes apache#4422. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
Add new property 'Cache Control' to allow user to set the cache-control http header on the S3 object. This property is not required, and has no default value. The implementation is similar to the Content-Type property, except that this property does not allow Expression Language. Update property description Add support EL for cache-control property This closes apache#4422. Signed-off-by: Peter Turcsanyi <turcsanyi@apache.org>
2 participants
Add new property 'Cache Control' to allow user to
set the cache-control http header on the S3 object.
This property is not required, and has no default value.
The implementation is similar to the Content-Type property,
except that this property does not allow Expression Language.