Adding mount path as env variable in container #7968

babugeet · 2024-06-07T11:20:50Z

Signed-off-by: babugeet abhinandhbg@gmail.com

Fixes #

#7963

Proposed Changes

Adding the mount path as Environment variable for container of pod spawned by the Job

Signed-off-by: babugeet <abhinandhbg@gmail.com>

knative-prow · 2024-06-07T11:20:59Z

Welcome @babugeet! It looks like this is your first PR to knative/eventing 🎉

knative-prow · 2024-06-07T11:20:59Z

Hi @babugeet. 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-sigs/prow repository.

Cali0707 · 2024-06-07T15:51:13Z

cmd/jobsink/main.go

+		job.Spec.Template.Spec.Containers[i].Env = append(job.Spec.Template.Spec.Containers[i].Env, corev1.EnvVar{
+			Name:  "K_EVENT_PATH",
+			Value: "/etc/jobsink-event",
+		})


The value you are using here isn't guaranteed to be correct. If you look at the logic above, a different volume mount may be chosen (which could have a different mount path)

@Cali0707 : i have updated the logic,
If the Volume mount already exists, the code is taking the volume mount path from the job
If the Volume mount is not there, the code is taking the defined mount path

Signed-off-by: "babugeet <abhinandhbg@gmail.com>"

Cali0707

/lgtm

Thanks @babugeet !

knative-prow · 2024-06-07T18:15:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: babugeet, Cali0707

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~~ [Cali0707]

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

Cali0707 · 2024-06-07T18:15:51Z

/ok-to-test

codecov · 2024-06-07T18:22:34Z

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.73%. Comparing base (9d11598) to head (673bc3a).
Report is 1 commits behind head on main.

Files	Patch %	Lines
cmd/jobsink/main.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7968      +/-   ##
==========================================
- Coverage   67.76%   67.73%   -0.03%     
==========================================
  Files         354      354              
  Lines       16505    16512       +7     
==========================================
  Hits        11184    11184              
- Misses       4632     4639       +7     
  Partials      689      689

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pierDipi · 2024-06-12T10:16:44Z

Thanks @babugeet for this! would you mind updating the documentation page to mention this new feature too ? https://github.com/knative/docs/blob/main/docs/eventing/sinks/job-sink.md

pierDipi · 2024-06-12T10:17:22Z

cmd/jobsink/main.go

+		job.Spec.Template.Spec.Containers[i].Env = append(job.Spec.Template.Spec.Containers[i].Env, corev1.EnvVar{
+			Name:  "K_EVENT_PATH",
+			Value: mountPathName,
+		})


This is an edge case but we need to check if the env variable is already defined or not here

Actually it was not mentioned, when existing code was checked

babugeet · 2024-06-12T12:02:26Z

Thanks @babugeet for this! would you mind updating the documentation page to mention this new feature too ? https://github.com/knative/docs/blob/main/docs/eventing/sinks/job-sink.md

Sure. Will raise a new PR for it. Can you pls share any similar sample release note PR, if you have handy

babugeet · 2024-06-13T16:09:37Z

Thanks @babugeet for this! would you mind updating the documentation page to mention this new feature too ? https://github.com/knative/docs/blob/main/docs/eventing/sinks/job-sink.md

Sure. Will raise a new PR for it. Can you pls share any similar sample release note PR, if you have handy

@Cali0707 can you pls help me to get a sample relase not PR

Cali0707 · 2024-06-13T16:33:56Z

@babugeet the original docs for this feature comes from: knative/docs#6005

Adding mount path as env variable in container

860900c

Signed-off-by: babugeet <abhinandhbg@gmail.com>

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 7, 2024

knative-prow bot requested review from Cali0707 and creydr June 7, 2024 11:21

Cali0707 reviewed Jun 7, 2024

View reviewed changes

Cali0707 requested a review from pierDipi June 7, 2024 15:51

Added enviornment variable based on the mountpath

673bc3a

Signed-off-by: "babugeet <abhinandhbg@gmail.com>"

Cali0707 approved these changes Jun 7, 2024

View reviewed changes

knative-prow bot assigned Cali0707 Jun 7, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2024

knative-prow bot merged commit 844dcbf into knative:main Jun 7, 2024
35 of 36 checks passed

pierDipi reviewed Jun 12, 2024

View reviewed changes

Cali0707 mentioned this pull request Jul 4, 2024

Add env variable to Job created by JobSink for the file path to the event #7963

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding mount path as env variable in container #7968

Adding mount path as env variable in container #7968

babugeet commented Jun 7, 2024

knative-prow bot commented Jun 7, 2024

knative-prow bot commented Jun 7, 2024

Cali0707 Jun 7, 2024

babugeet Jun 7, 2024

Cali0707 left a comment

knative-prow bot commented Jun 7, 2024

Cali0707 commented Jun 7, 2024

codecov bot commented Jun 7, 2024

pierDipi commented Jun 12, 2024 •

edited

Loading

pierDipi Jun 12, 2024 •

edited

Loading

babugeet Jun 12, 2024

babugeet commented Jun 12, 2024 •

edited

Loading

babugeet commented Jun 13, 2024

Cali0707 commented Jun 13, 2024

Adding mount path as env variable in container #7968

Adding mount path as env variable in container #7968

Conversation

babugeet commented Jun 7, 2024

Proposed Changes

knative-prow bot commented Jun 7, 2024

knative-prow bot commented Jun 7, 2024

Cali0707 Jun 7, 2024

Choose a reason for hiding this comment

babugeet Jun 7, 2024

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jun 7, 2024

Cali0707 commented Jun 7, 2024

codecov bot commented Jun 7, 2024

Codecov Report

pierDipi commented Jun 12, 2024 • edited Loading

pierDipi Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

babugeet Jun 12, 2024

Choose a reason for hiding this comment

babugeet commented Jun 12, 2024 • edited Loading

babugeet commented Jun 13, 2024

Cali0707 commented Jun 13, 2024

pierDipi commented Jun 12, 2024 •

edited

Loading

pierDipi Jun 12, 2024 •

edited

Loading

babugeet commented Jun 12, 2024 •

edited

Loading