-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix MarkUnsafe missing Spec in ansible operator #6376
Fix MarkUnsafe missing Spec in ansible operator #6376
Conversation
1fa5b6b
to
0e3fd58
Compare
0e3fd58
to
c792b48
Compare
I rebased the MR, in case you want to approve it. |
Hello, I'm sorry to ask, but could this MR be looked at please ? Its rather simple and is probably fast to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaelgatelement Thanks for this contribution! I apologize that the PR got lost in the weeds.
I have one question:
Going to close and reopen to rerun CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaelgatelement Just looked into the issue and seems like it was created long back in 2021. I remember on having a PR that marked the spec as unsafe if specified so in watches.yaml. Looks like it happens here: https://github.com/varshaprasad96/operator-sdk/blob/c247d4b4142cd7736f122796e3388fbd05f62b71/internal/ansible/runner/runner.go#L352-L354C4
where the parameters created from the spec are recursively marked unsafe. Just wondering if there was something missing even after that.
Indeed, this PR is a small fix to your feature. It seems the spec is not recursively marked as unsafe before the change I implemented here. EDIT : So i'm re-reading the code here, and I think mark unsafe was missing the magic |
Sorry for the ping, but because I edited my message, I'm not sure you received a notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After the discussion that has gone on and looking back at the referenced issue I think its reasonable for this change to come in. If we end up marking all the fields as unsafe anyways we might as well mark the spec unsafe also.
Thanks @gaelgatelement !
/lgtm
Got it! Thanks for fixing it @gaelgatelement! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Description of the change:
Closes #5160
Motivation for the change:
MarkUnsafe was missing some properties.