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

Fix MarkUnsafe missing Spec in ansible operator #6376

Merged

Conversation

gaelgatelement
Copy link
Contributor

Description of the change:
Closes #5160

Motivation for the change:
MarkUnsafe was missing some properties.

@gaelgatelement gaelgatelement changed the title Gaelg/mark spec unsafe Fix MarkUnsafe missing Spec in ansible operator Mar 23, 2023
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:05 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:05 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:05 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:09 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:09 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:09 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:09 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:10 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:10 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:10 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:10 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 23, 2023 17:10 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy March 24, 2023 07:25 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy June 13, 2023 07:47 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy June 13, 2023 07:47 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy June 13, 2023 07:47 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy June 13, 2023 07:47 — with GitHub Actions Inactive
@gaelgatelement gaelgatelement temporarily deployed to deploy June 13, 2023 07:47 — with GitHub Actions Inactive
@gaelgatelement
Copy link
Contributor Author

I rebased the MR, in case you want to approve it.

@gaelgatelement
Copy link
Contributor Author

Hello,

I'm sorry to ask, but could this MR be looked at please ? Its rather simple and is probably fast to review.

Copy link
Contributor

@everettraven everettraven left a 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:

internal/ansible/runner/runner.go Show resolved Hide resolved
@everettraven
Copy link
Contributor

Going to close and reopen to rerun CI

@everettraven everettraven reopened this Jul 24, 2023
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:18 — with GitHub Actions Inactive
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:18 — with GitHub Actions Inactive
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:18 — with GitHub Actions Inactive
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:18 — with GitHub Actions Inactive
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:19 — with GitHub Actions Inactive
@everettraven everettraven temporarily deployed to deploy July 24, 2023 13:19 — with GitHub Actions Inactive
Copy link
Member

@varshaprasad96 varshaprasad96 left a 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.

@gaelgatelement
Copy link
Contributor Author

gaelgatelement commented Jul 24, 2023

@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 _spec variable. Ansible tries to parse the content using jinja templating since its not marked as unsafe.

@gaelgatelement
Copy link
Contributor Author

Sorry for the ping, but because I edited my message, I'm not sure you received a notification.

Copy link
Contributor

@everettraven everettraven left a 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

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2023
@varshaprasad96
Copy link
Member

EDIT : So i'm re-reading the code here, and I think mark unsafe was missing the magic _spec variable. Ansible tries to parse the content using jinja templating since its not marked as unsafe.

Got it! Thanks for fixing it @gaelgatelement!

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@varshaprasad96 varshaprasad96 merged commit b8a271a into operator-framework:master Jul 26, 2023
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Ansible "markUnsafe" option should mark more things as unsafe
3 participants