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

Markers shown on Spring application.yml #52

Merged
merged 11 commits into from
Aug 7, 2024

Conversation

timtebeek
Copy link
Contributor

@timtebeek timtebeek commented Aug 2, 2024

@timtebeek timtebeek requested a review from sambsnyd August 2, 2024 22:54
@timtebeek timtebeek self-assigned this Aug 2, 2024
@timtebeek timtebeek added bug Something isn't working enhancement New feature or request labels Aug 2, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor Author

timtebeek commented Aug 2, 2024

Figured explore traits for rewrite-kubernetes, as the KubernetesParser had been a bit of an odd one up to now. It looks like we're not using the KubernetesParser anywhere, and with Traits recently introduced that seemed the way to go.

I've for now only converted this one recipe that was too eager matching just any yaml file to find missing patterns. With these changes it now only matches Kubernetes resources with at least some kind defined, reusing the older UpdateKubernetesModel visitor, but deprecating the KubernetesParser and KubernetesVisitor, as well as the helper methods in K8s that are perhaps best converted in the future.

I'd welcome a review on the approach as well as implementation details; this was mostly to familiarize myself with Traits such that I can better guide and shape community contributions going forward.

One slight concern with this approach is that we call UpdateKubernetesModel more often, as opposed just once when parsing. But using the KubernetesParser would require us to change the OSS plugins & CLI, which is likely to become troublesome to maintain longer term if we repeat that approach for other yaml based standards like CI systems.

@timtebeek timtebeek marked this pull request as ready for review August 2, 2024 23:00
@timtebeek timtebeek requested a review from pstreef August 6, 2024 12:10
@pstreef
Copy link

pstreef commented Aug 7, 2024

I don't really know what Traits are yet so I cannot really give much insight here..

@timtebeek
Copy link
Contributor Author

I don't really know what Traits are yet so I cannot really give much insight here..

Ah thanks for having a brief look; then we'll just assume this is ok to merge, as the tests have been expanded, and I imagine Sam is busy enough as it is wrapping up before his holiday. We can always revise if needed; the contract of the recipes exposed hasn't changed.

@timtebeek timtebeek merged commit b711be6 into main Aug 7, 2024
2 checks passed
@timtebeek timtebeek deleted the kubernetes-markers-on-spring-yml branch August 7, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Markers are left in all yaml files when running recipes with maven or gradle plugin
2 participants