Make in-place sed operation portable#2303
Make in-place sed operation portable#2303istio-merge-robot merged 2 commits intoistio:masterfrom sdake:fix_update_versions
Conversation
sed -i is not portable. GNU sed and Darwin have different operational behaviors. This would not be a problem, however, homebrew is used occassionally on Darwin, which brings in GNU sed.
|
Hi @sdake. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Helm values.yaml does not properly override the proxy tag with -a.
Codecov Report
@@ Coverage Diff @@
## master #2303 +/- ##
==========================================
- Coverage 78.39% 78.35% -0.04%
==========================================
Files 77 79 +2
Lines 7577 7680 +103
==========================================
+ Hits 5940 6018 +78
- Misses 1327 1341 +14
- Partials 310 321 +11
Continue to review full report at Codecov.
|
|
@sdake you can check running OS and then use Darwin's version of sed. When you move files you overwrite original file properties. |
|
That works just fine unless someone has installed homebrew, in which case, the following issue occurs: istio/old_issues_repo#139 |
|
/assign @ldemailly |
|
@sdake I see your point, I still think overwriting files properties is not right thing to do, but I would let community to decide. |
|
you raise a valid point related to file properties. I'll see if there is a way to keep properties intact. which properties concern you most? those shown with stat? Cheers |
|
@sdake After checking the changes again, I see that the operation is done mostly on files located in temp dir with the exception of values.yaml which is not big deal. thanks. |
|
Lars mentioned cat a > b works to preserve properties - so I'll verify that and push a patch later today if it works (or report back that it doesn't). |
|
/lgtm thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ldemailly The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test istio-presubmit |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
|
If the file is headed for the bitbucket (which IMO is a good idea 👍 - no need to gold plate it with keeping properties intact. Cheers |
sed -i is not portable. GNU sed and Darwin have different
operational behaviors. This would not be a problem, however,
homebrew is used occassionally on Darwin, which brings in
GNU sed.