Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Closes #2262: Allowing users to specify arbitrary CLI arguments for Ansible-based operators #3374
Closes #2262: Allowing users to specify arbitrary CLI arguments for Ansible-based operators #3374
Changes from 8 commits
71814cc
6cdddd7
af1eab2
e5f1205
0da45dd
c61ddf7
6b10eac
05e3a44
8303ef9
e35d950
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
nit space at the end.
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.
Regards the
addFile
:log.Error(err, "Unable to write file", "Path", fullPath)
we can replace by:0644
needs to be replaced by the const in the projectutils. See: https://github.com/operator-framework/operator-sdk/blob/master/internal/util/projutil/project_util.go#L31There 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.
The reason here is because the addFile just do the above code so has no need for an extra func and then, it makes easier understand that it is about to write a file in the disk.
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.
All the parameters above (Parameters, EnvVars, Settings) all use the addFile while having their bytes written to disk, so I thought we could use that function instead of repeating the code.
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.
Hi @VenkatRamaraju,
Thank you for your reply. Let's see the addFile:
operator-sdk/internal/ansible/runner/internal/inputdir/inputdir.go
Lines 55 to 63 in 8303ef9
Is not it the same of call the
WriteFile
?. So, why we need that?So my suggestion is to replace the addFiles calls to use
ioutil.WriteFile
directly.And then, see that is better we use the FileMode const;
operator-sdk/internal/util/projutil/project_util.go
Line 31 in 8303ef9
Feel free to ping me as well for we spoke about it.
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.
@camilamacedo86 since this is just reusing the function the rest of the code is already using I think dropping the
addFile
function would make sense to do separately, I don't think it's worth blocking the PR to fix it hereThere 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.
Yep .. I just saw that.
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.
This configmap needs a
data
field or it will just be empty