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

Provide backward compatible way to globally set permitted classes #531

Conversation

jrafanie
Copy link
Member

This supports rails 7 and 6.1.

Similar to the change in: ManageIQ/manageiq#22887

This supports rails 7 and 6.1.

Similar to the change in: ManageIQ/manageiq#22887
@miq-bot
Copy link
Member

miq-bot commented Jun 28, 2024

Checked commit jrafanie@5a6dfc0 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2024

Minor, but can we

  1. move this into a spec/support file in core
  2. use that shared spec/support method here
  3. Put a note in the spec/support entry that somehow let's us find it later (maybe a comment about the Rails version or something that we can search on).

My main concerns are duplication of code and then being able to find all those later when we need to undo it.

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2024

Actually i said spec/support but the original code is in yaml_permitted_classes, so maybe it's just easy to refactor into another method on that module and use it here. Just trying to avoid the duplication.

@jrafanie
Copy link
Member Author

Minor, but can we

  1. move this into a spec/support file in core
  2. use that shared spec/support method here
  3. Put a note in the spec/support entry that somehow let's us find it later (maybe a comment about the Rails version or something that we can search on).

My main concerns are duplication of code and then being able to find all those later when we need to undo it.

Sure, I'll add a follow up item to the rails 7 todo list to find a generic way for all of the various repos to do the same function. Or maybe just delete the rails 6.1 way when it's dropped. Currently debugging tough api and ui-classic test failures so cleanup is less priority.

@jrafanie
Copy link
Member Author

ok, I added a followup item to ManageIQ/manageiq#22052

@jrafanie
Copy link
Member Author

jrafanie commented Jul 9, 2024

@Fryguy this is ready to go. I'll come back and clean these up in the follow task: ManageIQ/manageiq#22052. When we drop rails 6.1, it should be easy to mass remove them.

@Fryguy Fryguy merged commit 87f7057 into ManageIQ:master Jul 9, 2024
2 checks passed
@jrafanie jrafanie deleted the rails7_rails6_friendly_yaml_permitted_classes branch July 18, 2024 14:49
jrafanie added a commit to jrafanie/manageiq-providers-kubernetes that referenced this pull request Aug 6, 2024
Drops the rails 6.1 compatibility from:
ManageIQ#531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants