Skip to content
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

Fix applyNode GraphQL API #307

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Fix applyNode GraphQL API #307

merged 1 commit into from
Sep 23, 2024

Conversation

sophie-cluml
Copy link
Contributor

close #306
close #303

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 80.87649% with 48 lines in your changes missing coverage. Please review.

Project coverage is 67.23%. Comparing base (5d2a9d0) to head (fad1558).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/graphql/node/control.rs 80.87% 48 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
+ Coverage   67.06%   67.23%   +0.17%     
==========================================
  Files          68       67       -1     
  Lines       12986    13142     +156     
==========================================
+ Hits         8709     8836     +127     
- Misses       4277     4306      +29     

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

@sophie-cluml sophie-cluml force-pushed the sophie/fix-apply-node branch 3 times, most recently from df63ea4 to 0d5dee3 Compare September 19, 2024 12:31
}

Ok(ApplyNodeResponse { id, giganto_draft })
}
}

fn format_agent_updates(node: &Node, target_agents: &NotificationTarget) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why the log message includes all the content of the agents' configs and drafts? Some of these items are usually not visible to users, as the UI only shows what can be modified by them, but they become visible because of this message. If you're considering keeping a history of all the changes, I think it should be stored in the database rather than in log messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log was created to follow this guide

[Central server] logs the apply actions along with settings config, settings_draft draft, and the timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I wrote the sentence. I apologize for the confusion caused by using the term "log" too loosely. When I used "log," I was thinking of its literal meaning—saving information—rather than in the syslog style. I intended that the specific method of storing could be decided based on the context. In this case, I don’t think the configs and drafts should be written to the log. As for how to handle them, I believe we can decide that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs on config and draft are removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs on config and draft are removed.

I still see them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for the mistake. 🙇 It's done from my side now.

@sehkone sehkone merged commit 947f677 into main Sep 23, 2024
10 checks passed
@sehkone sehkone deleted the sophie/fix-apply-node branch September 23, 2024 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants