refactor: To ensure telemetry is only enabled when both conditions are met. #393

Carpe-Wang · 2024-05-17T14:13:13Z

Description

About #339. Based on my understanding of this issue, to ensure telemetry is only enabled when both conditions are met, thus avoiding unnecessary or incorrect configuration leading to resource waste or incorrect data collection, I believe we need to add a check for enableTelemetry being true in the if applicationInsightsID != ""{ and I want to change to if applicationInsightsID != "" && enableTelemetry {. But I'm not sure if my understanding is correct.

Related Issue

If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.

Checklist

I have read the contributing documentation.
I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
I have correctly attributed the author(s) of the code.
I have tested the changes locally.
I have followed the project's style guidelines.
I have updated the documentation, if necessary.
I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Please add any relevant screenshots or GIFs to showcase the changes made.

Additional Notes

Add any additional notes or context about the pull request here.

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

rbtr · 2024-05-17T18:38:36Z

@Carpe-Wang this seems correct, can you test it and confirm the behavior is what we want?
Also need you to comment on #339 if you want it assigned to you 🙂

Carpe-Wang · 2024-05-18T09:01:22Z

@microsoft-github-policy-service agree

Carpe-Wang · 2024-05-19T11:37:43Z

@Carpe-Wang this seems correct, can you test it and confirm the behavior is what we want? Also need you to comment on #339 if you want it assigned to you 🙂

hey @rbtr, I had comment on #339, and I noticed #339 had assigned to me. I make some new commits about test the unit test successfully.

Carpe-Wang · 2024-05-21T12:30:32Z

Hey, I have a question about how to execute the workflows whose status is always Expected — Waiting for status to be reported.

rbtr · 2024-05-21T17:06:56Z

Only project maintainers can trigger the workflows

init/retina/main_linux.go

nddq

@Carpe-Wang can you sign all your commits please, thanks 🙂

Carpe-Wang · 2024-06-08T06:47:04Z

@Carpe-Wang can you sign all your commits please, thanks 🙂

@nddq I have signed all my commits. I hope they meet your expectations. Please check them.

nddq · 2024-06-09T04:35:41Z

@Carpe-Wang seems like you missed a few commits

Carpe-Wang · 2024-06-09T04:44:30Z

@Carpe-Wang seems like you missed a few commits
@nddq yeah, those commits were unverified because the email in my local Git configuration didn't match the email for GPG. So, I deleted them and updated the email in my local Git configuration. However, the final code is consistent.

nddq · 2024-06-10T17:21:42Z

@Carpe-Wang seems like you missed a few commits
@nddq yeah, those commits were unverified because the email in my local Git configuration didn't match the email for GPG. So, I deleted them and updated the email in my local Git configuration. However, the final code is consistent.

So what I meant is that we are looking for the Verified check across all commits, as you can see, some of them are missing that

My suggestion is that you could squash all the previous commit into a single one, sign it and the force push it to this branch 🙂 Let me know if you have any issue

nddq · 2024-06-18T13:49:47Z

@Carpe-Wang there are still unsigned commits, so we can't merge this yet 🥲

rbtr · 2024-06-18T16:20:11Z

@Carpe-Wang as mentioned previously, all commits in a PR must be signed. As long as you have commits without a (Verified) badge on this page, this will not be mergeable due to signing policy. Updating the branch from main won't change this.
The easy way to resolve this is to squash the 21 commits currently on your branch down to a single one. Do this via interactive rebase, eg: git rebase -i HEAD~21, leave the first one as "pick", change all the rest to "squash", and adjust the commit message appropriately. Then force push the branch.

…e met. Signed-off-by: Carpe-Wang <[email protected]>

Carpe-Wang · 2024-06-19T05:43:11Z

@nddq Sorry, I've been a bit busy these days and didn't have enough time to handle it. I just did a reset --hard now. If this doesn't meet your merge requirements, I'll make the changes again.

Carpe-Wang · 2024-06-19T05:44:57Z

@rbtr thank you for your comment. I haven't used rebase before. I tried a few times without much success because when I pushed, there were many commits that weren't mine. So, I opted to use reset --hard directly. Thank you for your guidance.

nddq · 2024-06-19T15:02:22Z

init/retina/main_linux.go

+	}
+
+	// Define a command-line flag "config" with a default value and parse the flags, then create a logger with a name and version.
+	configPath := flag.String("config", "/retina/config/config.yaml", "path to the config file")


@Carpe-Wang looks like you need to pass in the path to the config via arguments to the init-container as well. Please refer to how we are currently doing it for the agent's main container

I added the following section to the relevant file:

args: - --config=/retina/config/config.yaml

Does this meet your expectation?"

that should work

okay, I had push the newest commit.

oops i forgot to mention, you will also have to mount the path the config file like we are doing with the main container. Thanks for the prompt response!

@nddq I change the file according to your comment. Please review it.

Signed-off-by: Carpe-Wang <[email protected]>

github-actions · 2024-07-27T00:18:35Z

This PR will be closed in 7 days due to inactivity.

github-actions · 2024-08-03T00:18:49Z

Pull request closed due to inactivity.

Carpe-Wang requested a review from a team as a code owner May 17, 2024 14:13

Carpe-Wang requested review from jimassa and matmerr May 17, 2024 14:13

nddq linked an issue May 20, 2024 that may be closed by this pull request

ideally agent's init container enables telemetry based on ConfigMap flag #339

Closed

Carpe-Wang changed the title ~~To ensure telemetry is only enabled when both conditions are met.~~ refactor : To ensure telemetry is only enabled when both conditions are met. May 23, 2024

rbtr changed the title ~~refactor : To ensure telemetry is only enabled when both conditions are met.~~ refactor: To ensure telemetry is only enabled when both conditions are met. May 23, 2024

nddq requested changes May 31, 2024

View reviewed changes

init/retina/main_linux.go Outdated Show resolved Hide resolved

nddq reviewed Jun 7, 2024

View reviewed changes

init/retina/main_linux.go Outdated Show resolved Hide resolved

nddq approved these changes Jun 8, 2024

View reviewed changes

nddq requested changes Jun 8, 2024

View reviewed changes

Carpe-Wang force-pushed the main branch from a0ae94e to 3d833e7 Compare June 8, 2024 06:40

nddq previously approved these changes Jun 9, 2024

View reviewed changes

Carpe-Wang dismissed nddq’s stale review via 6ead12f June 11, 2024 04:20

Carpe-Wang force-pushed the main branch 4 times, most recently from eaf88d6 to cb11c07 Compare June 11, 2024 04:57

nddq enabled auto-merge June 11, 2024 15:47

nddq previously approved these changes Jun 11, 2024

View reviewed changes

auto-merge was automatically disabled June 19, 2024 05:38
Head branch was pushed to by a user without write access

Carpe-Wang dismissed nddq’s stale review via 385b81c June 19, 2024 05:38

Carpe-Wang force-pushed the main branch from d97effc to 385b81c Compare June 19, 2024 05:38

refactor: To ensure telemetry is only enabled when both conditions ar…

ffea69f

…e met. Signed-off-by: Carpe-Wang <[email protected]>

Carpe-Wang force-pushed the main branch from 385b81c to ffea69f Compare June 19, 2024 05:40

Merge branch 'main' into main

650e718

nddq approved these changes Jun 19, 2024

View reviewed changes

nddq enabled auto-merge June 19, 2024 14:02

nddq added this pull request to the merge queue Jun 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2024

nddq added this pull request to the merge queue Jun 19, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 19, 2024

nddq requested changes Jun 19, 2024

View reviewed changes

Carpe-Wang and others added 3 commits June 20, 2024 11:40

Merge branch 'main' into main

16864db

the path to the config via arguments to the init-container

918ba06

Signed-off-by: Carpe-Wang <[email protected]>

the path to the config via arguments to the init-container

c4665af

Signed-off-by: Carpe-Wang <[email protected]>

github-actions bot added the meta/waiting-for-author Blocked and waiting on the author label Jul 27, 2024

github-actions bot closed this Aug 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: To ensure telemetry is only enabled when both conditions are met. #393

refactor: To ensure telemetry is only enabled when both conditions are met. #393

Carpe-Wang commented May 17, 2024 •

edited

Loading

rbtr commented May 17, 2024

Carpe-Wang commented May 18, 2024

Carpe-Wang commented May 19, 2024

Carpe-Wang commented May 21, 2024

rbtr commented May 21, 2024

nddq left a comment

Carpe-Wang commented Jun 8, 2024

nddq commented Jun 9, 2024

Carpe-Wang commented Jun 9, 2024

nddq commented Jun 10, 2024

nddq commented Jun 18, 2024

rbtr commented Jun 18, 2024 •

edited

Loading

Carpe-Wang commented Jun 19, 2024

Carpe-Wang commented Jun 19, 2024

nddq Jun 19, 2024

Carpe-Wang Jun 19, 2024

nddq Jun 19, 2024

Carpe-Wang Jun 20, 2024

nddq Jun 21, 2024

Carpe-Wang Jun 26, 2024

github-actions bot commented Jul 27, 2024

github-actions bot commented Aug 3, 2024

refactor: To ensure telemetry is only enabled when both conditions are met. #393

refactor: To ensure telemetry is only enabled when both conditions are met. #393

Conversation

Carpe-Wang commented May 17, 2024 • edited Loading

Description

Related Issue

Checklist

Screenshots (if applicable) or Testing Completed

Additional Notes

rbtr commented May 17, 2024

Carpe-Wang commented May 18, 2024

Carpe-Wang commented May 19, 2024

Carpe-Wang commented May 21, 2024

rbtr commented May 21, 2024

nddq left a comment

Choose a reason for hiding this comment

Carpe-Wang commented Jun 8, 2024

nddq commented Jun 9, 2024

Carpe-Wang commented Jun 9, 2024

nddq commented Jun 10, 2024

nddq commented Jun 18, 2024

rbtr commented Jun 18, 2024 • edited Loading

Carpe-Wang commented Jun 19, 2024

Carpe-Wang commented Jun 19, 2024

nddq Jun 19, 2024

Choose a reason for hiding this comment

Carpe-Wang Jun 19, 2024

Choose a reason for hiding this comment

nddq Jun 19, 2024

Choose a reason for hiding this comment

Carpe-Wang Jun 20, 2024

Choose a reason for hiding this comment

nddq Jun 21, 2024

Choose a reason for hiding this comment

Carpe-Wang Jun 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 27, 2024

github-actions bot commented Aug 3, 2024

Carpe-Wang commented May 17, 2024 •

edited

Loading

rbtr commented Jun 18, 2024 •

edited

Loading