Skip to content

2.7: Changes to Auto-configuration #156 #182

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

Merged

Conversation

ijusti
Copy link
Contributor

@ijusti ijusti commented Jun 30, 2022

No description provided.

@pivotal-cla
Copy link

@ijusti Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@ijusti Thank you for signing the Contributor License Agreement!

@fabapp2
Copy link
Contributor

fabapp2 commented Jul 7, 2022

Hi @ijusti!
I've been on PTO thus the long delay.
Thanks a lot for your contribution and the many beautifications and fixes. Very much appreciated! 🙏
I had a few comments regarding the operation on files and would ask you to provide at least a test for the Action itself.
If you could resolve these comments I'll happily merge your PR.
Thanks again!

@fabapp2 fabapp2 added the 2.7.0 label Jul 7, 2022
@fabapp2 fabapp2 added this to the Spring Boot Upgrade 3.0 (M3) milestone Jul 7, 2022
}

@Override
public boolean evaluate(ProjectContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use e.g. PathPatternMatchingProjectResourceFinder instead?
This would also remove the dependency from Condition to Action and remove the assumption that the resources in the ProjectContext exist as files.

return !context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/spring.factories")).isEmpty();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i need to check that near spring.factories no new file exists

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, that's a good idea. What about something like this?

return !context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/spring.factories")).isEmpty() && 
context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/org.springframework.boot.autoconfigure.AutoConfiguration.imports")).isEmpty()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this good, and I tried to make this way? but there is no spring.factories in context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

something like this, that mean that there is no new imports near spring.factories

return context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/spring.factories"))
.stream()
.anyMatch(resource-> context.search(new PathPatternMatchingProjectResourceFinder("resource.path.parent +"org.springframework.boot.autoconfigure.AutoConfiguration.imports")).isEmpty())

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I got your valid point here.
I am thinking that if we replace all old with new factories in one go and remove the old ones (or move with MoveFilesAction), there should not be both (old and new) in one project. If this is "guaranteed" we can skip the second part of the check?

You could replace resource.path.parent with resource.getAbsoluetPath().getParent().resolve("org.springframework.boot.autoconfigure.AutoConfiguration.imports") and then use AbsolutePathResourceFinder with this path.

import static java.nio.file.StandardOpenOption.WRITE;
import static java.util.function.Predicate.not;

public class CreateAutoconfigurationAction extends AbstractAction {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please provide a test for this class.
You can create a synthetic ProjectContext with TestProjectContext helper class.
Run the Action against the created ProjectConetxt and verify the result.
You can have a look at existing tests for examples.

@fabapp2 fabapp2 linked an issue Jul 7, 2022 that may be closed by this pull request
}

@Override
public boolean evaluate(ProjectContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, that's a good idea. What about something like this?

return !context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/spring.factories")).isEmpty() && 
context.search(new PathPatternMatchingProjectResourceFinder("**/src/main/resources/META-INF/org.springframework.boot.autoconfigure.AutoConfiguration.imports")).isEmpty()

@fabapp2
Copy link
Contributor

fabapp2 commented Jul 14, 2022

Hi @ijusti !
Could you provide the requested changes?
I am sorry if I am pushy here but we'll need this issue (very) soon.
It's no problem if you don't have time, I'd merge what you did and add the requested changes myself then (crediting you as author).
Let me know what works for you?

@ijusti ijusti force-pushed the sb27-autoconfiguration branch from 9a0d8bc to 9a81c94 Compare July 20, 2022 15:24
@ijusti ijusti force-pushed the sb27-autoconfiguration branch from 9a81c94 to 8413ea1 Compare July 20, 2022 15:51
@fabapp2 fabapp2 merged commit 6bce7ae into spring-projects-experimental:main Jul 21, 2022
@fabapp2 fabapp2 added type: enhancement New feature or request upgrade:boot labels Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2.7: Changes to Auto-configuration
3 participants