Add option to allow enviornment variables using file

[boaz0]

Description

This PR gives the option to set environment variables from a file by specifying its path in --env-file.

Changes

• Add GetEnvsFromFile to util
• Add UpdateEnvsFromFile to podspec_helpers
• Add to podspec flags --env-file and EnvFile to PodSpec.
• Run UpdateEnvsFromFile if env-file was changed.

Reference

Closes #278

[google-cla]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[knative-prow-robot]

Welcome @boaz0! It looks like this is your first PR to knative/client 🎉

[knative-prow-robot]

Hi @boaz0. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

[boaz0]

@googlebot I fixed it.

[itsmurugappan]

/ok-to-test

[itsmurugappan]

Thanks for the PR. At first glance, I noticed that unit tests were missing. Can you please add some tests while we review the PR. Thanks

[dsimansk]

Can you please rerun codegen and commit the changes? And as Muru mentioned that'd nice to have unit tests, otherwise the PR won't pass Codecov checks.

./hack/build.sh -c

[rhuss]

@boaz0 thanks a lot for the PR and your contribution. There are some minor comments and suggestions to the PR. Would you have time to address those? No problem if not, we would then help you here.

[maximilien]

Please address the go format changes as it makes reviewing a bit painful (first world problem I know) and please add some tests (unit and integration)

Thanks for contribution

[boaz0]

@maximilien @rhuss @itsmurugappan @dsimansk I updated the PR as you requested. Feel free to give a second code-review.
Thanks.

[itsmurugappan]

@rhuss @dsimansk why are we iterating the whole array of args here ->

client/pkg/kn/flags/podspec_helper.go

Line 70 in dedcad7

for _, arg := range allArgs {

It could simplify this pr if its not there.

[itsmurugappan]

except for some small things, looks good. Lets wait for @rhuss or @dsimansk to get back on my question above.

[codecov]

Codecov Report

Merging #1433 (ada3c44) into main (e89bdc7) will decrease coverage by 0.28%.
The diff coverage is 75.00%.

❗ Current head ada3c44 differs from pull request most recent head 8cf886f. Consider uploading reports for the commit 8cf886f to get more accurate results

@@            Coverage Diff             @@
##             main    #1433      +/-   ##
==========================================
- Coverage   78.98%   78.69%   -0.29%     
==========================================
  Files         162      162              
  Lines        8416     8392      -24     
==========================================
- Hits         6647     6604      -43     
- Misses       1088     1101      +13     
- Partials      681      687       +6     

Impacted Files	Coverage Δ
pkg/kn/flags/podspec.go	75.15% <57.14%> (-1.67%)	⬇️
pkg/util/parsing_helper.go	91.56% <100.00%> (+1.15%)	⬆️
pkg/eventing/v1/client.go	82.67% <0.00%> (-4.65%)	⬇️
pkg/kn/commands/trigger/update.go	72.72% <0.00%> (-4.47%)	⬇️
pkg/sources/v1/container_client.go	83.72% <0.00%> (-4.42%)	⬇️
pkg/serving/v1alpha1/client.go	82.69% <0.00%> (-4.08%)	⬇️
pkg/sources/v1beta2/ping_client.go	80.00% <0.00%> (-3.96%)	⬇️
pkg/kn/commands/source/container/update.go	86.95% <0.00%> (-3.52%)	⬇️
pkg/kn/commands/subscription/update.go	65.95% <0.00%> (-2.94%)	⬇️
pkg/sources/v1beta2/ping_client_mock.go	75.00% <0.00%> (-1.00%)	⬇️
... and 7 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e89bdc7...8cf886f. Read the comment docs.

[rhuss]

@rhuss @dsimansk why are we iterating the whole array of args here ->

client/pkg/kn/flags/podspec_helper.go

Line 70 in dedcad7

for _, arg := range allArgs {

It could simplify this pr if its not there.

afair its so to keep the order of the environment variables as specified (a simple map is not good enough). haven't check this PR yet, but we should also take care that mixing --env with --env-from does also respect the order as provided on the commandline. (i.e. -e A=1 -env-from myenv.txt -e B=2 should keep the there order A, all from the file, B)

[itsmurugappan]

@boaz0 sorry for the delay. Here is the summary of changes

• add another || and handle env-file logic here
• Pass the envfile values to update and remove to this func
• In the above func, the args are iterated to keep the order, so pls add an additional check similar to this for env file. When this condition is satisfied, flush out all the env variables from env file to update iterator. (to keep the order)

Please let me know if you have any questions.

[boaz0]

@itsmurugappan thanks for the guidance. I updated the PR. I hope it's good enough.
Thanks.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/flags/podspec.go	84.3%	84.0%	-0.2
pkg/kn/flags/podspec_helper.go	84.6%	85.0%	0.4
pkg/util/parsing_helper.go	90.7%	91.9%	1.2

[itsmurugappan]

changes look good to me, the only thing is code coverage is reduced so the pr merge might not succeed. @rhuss @maximilien @dsimansk can you please approve this pr, so it would allow the additional workflows to run.

[rhuss]

/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: boaz0, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhuss]

@itsmurugappan the error looks strange, like a commit with an improper email. Would it be possible to squash the PR manually and force push it, to get rid of this ?

-->

[dsimansk]

There was once a very similar error in the kamelet repository. rebase to main should get rid of it.

[rhuss]

/lgtm

[boaz0]

Sorry for the late response (holidays and family...).
Thanks for the opportunity to work on this and for the guidance @rhuss @dsimansk @itsmurugappan. Now moving to the next issue.