Skip to content

Add configuration-strategy property to RedisSessionProperties to configure ConfigureRedisAction #17022

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add configuration-strategy property to RedisSessionProperties to configure ConfigureRedisAction #17022

wants to merge 1 commit into from

Conversation

nosan
Copy link
Contributor

@nosan nosan commented May 30, 2019

see gh-16943

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 30, 2019
snicoll
snicoll reviewed Jun 3, 2019
View reviewed changes
Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PR. I don't think action is the right term for this and I've made a few suggestions. Flagging for team attention to see what the rest of the team thinks.

...ure/src/main/java/org/springframework/boot/autoconfigure/session/RedisSessionProperties.java Outdated
/**
* Allows specifying a strategy for configuring and validating Redis.
*/
private RedisAction action = RedisAction.NOTIFY_KEYSPACE_EVENTS;
Copy link
Member

@snicoll snicoll Jun 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface is quite generic and allows the user to do anything they want to configure and validate the connection. I don't think "action" is the right term for this as what we set is a "configuration and validation strategy".

configurationStrategy is a bit mouthful but more explicit perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having in mind the purpose of ConfigureRedisAction, naming this property configurationStrategy would be more appropriate.

...ure/src/main/java/org/springframework/boot/autoconfigure/session/RedisSessionProperties.java Outdated
/**
* Allows specifying a strategy for configuring and validating Redis.
*/
public enum RedisAction {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigurationStrategy would be the logical name there but it fees a bit odd so a bit more brainstorming is required I think.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jun 3, 2019
vpavic
vpavic reviewed Jun 3, 2019
View reviewed changes
Copy link
Contributor

@vpavic vpavic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd hold this off for a while because we at the Spring Session side should probably take a closer look at our support for configuring ConfigureRedisAction.

@nosan nosan changed the title Add action property to RedisSessionProperties to configure ConfigureRedisAction Add configuration-strategy property to RedisSessionProperties to configure ConfigureRedisAction Jun 3, 2019
@nosan
Copy link
Contributor Author

nosan commented Jun 3, 2019

@snicoll @vpavic
Thank you for your feedback.

PR has been updated.

@nosan nosan force-pushed the gh-16943 branch 2 times, most recently from 90ba85a to 866f97c Compare June 4, 2019 12:13
@nosan nosan force-pushed the gh-16943 branch 2 times, most recently from 3fde228 to 2f1f353 Compare June 10, 2019 13:16
@philwebb philwebb self-assigned this Jun 12, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jun 12, 2019
@philwebb philwebb added this to the 2.2.x milestone Jun 12, 2019
philwebb pushed a commit that referenced this pull request Jun 13, 2019
@nosan @philwebb
Add session property for ConfigureRedisAction
a0fabc9 
Add a new property to `RedisSessionProperties` that allows the default
`ConfigureRedisAction` to be changed. Users can still also configure
the action using a `@Bean`.

See gh-17022
philwebb added a commit that referenced this pull request Jun 13, 2019
@philwebb
Polish "Add session property for ConfigureRedisAction"
919913a 
See gh-17022
@philwebb philwebb closed this in 1213654 Jun 13, 2019
philwebb added a commit that referenced this pull request Jun 13, 2019
@philwebb
Fix incomplete polish of RedisSessionProperties
e0aa90c 
Fix polish commit from 919913a to correctly name the property
getter/setters.

See gh-17022
@philwebb
Copy link
Member

Thanks for another contribution @nosan!

I went back and forth on the property name a few times and ended up with configure-action in the end. It's a hard one to name because we ideally want some clues that it relates to configuring the Redis server, but we also want to indicate that it's us doing the configuration and to also somehow link back to the ConfigureRedisAction interface. I also managed to mess up the polish commit so there's an additional one outside of the merge commit.

@snicoll Let me know if you think the name might still be a problem. We still have time to refine it.

@philwebb
Copy link
Member

I'd hold this off for a while because we at the Spring Session side should probably take a closer look at our support for configuring ConfigureRedisAction.

Sorry @vpavic, I missed this comment until it was too late. Hopefully whatever changes you've got in mind won't be too hard to apply to the auto-configuration once they're ready. Is there a Spring Session issue we should be tracking?

@wilkinsona wilkinsona modified the milestones: 2.2.x, 2.2.0.M4 Jun 13, 2019
@nosan nosan deleted the gh-16943 branch June 13, 2019 13:18
@vpavic vpavic mentioned this pull request Jun 17, 2019
Open
@vpavic
Copy link
Contributor

vpavic commented Jun 17, 2019

No problem @philwebb. I've opened spring-projects/spring-session#1458 to consider doing something about this on Spring Session side.

@izeye izeye mentioned this pull request Jun 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snicoll snicoll snicoll left review comments

@vpavic vpavic vpavic left review comments

Assignees

@philwebb philwebb

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.2.0.M4
Development

Successfully merging this pull request may close these issues.
6 participants
@nosan @philwebb @vpavic @snicoll @wilkinsona @spring-projects-issues