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

Support command-line interface and update configuration API #796

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

BLYKIM
Copy link
Contributor

@BLYKIM BLYKIM commented Aug 9, 2024

  • Support command line interface.
    • Remove cert, key, root fields from config file.
  • Change setGigantoConfig to receive toml-formatted string with full configuration.
  • Update gigantoConfig to respond full configuration.
  • Rename root to ca_certs.

Closes: #779
Closes: #784
Closes: #785
Closes: #807

src/settings.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
@BLYKIM BLYKIM force-pushed the bly/command_line branch 4 times, most recently from fcb6bd3 to 55b608e Compare August 13, 2024 22:35
Cargo.toml Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
src/graphql/status.rs Outdated Show resolved Hide resolved
src/graphql/status.rs Outdated Show resolved Hide resolved
@kimhanbeom
Copy link
Contributor

kimhanbeom commented Aug 22, 2024

@BLYKIM There is currently further discussion about whether root should be renamed to root_ca or something else, so this pr should be pending.

src/graphql/status.rs Show resolved Hide resolved
src/graphql/status.rs Outdated Show resolved Hide resolved
@BLYKIM BLYKIM force-pushed the bly/command_line branch 2 times, most recently from 402a536 to 17f42ff Compare August 27, 2024 06:13
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@BLYKIM BLYKIM force-pushed the bly/command_line branch 2 times, most recently from 8d7630c to feb0331 Compare August 28, 2024 01:53
@sophie-cluml
Copy link
Contributor

#796 (comment) 에 대하여 #819 이슈가 생성되었습니다.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 85.25180% with 41 lines in your changes missing coverage. Please review.

Project coverage is 76.83%. Comparing base (1469d17) to head (c467297).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/main.rs 35.00% 26 Missing ⚠️
src/settings.rs 6.25% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   76.06%   76.83%   +0.76%     
==========================================
  Files          31       31              
  Lines       24909    24965      +56     
==========================================
+ Hits        18948    19181     +233     
+ Misses       5961     5784     -177     

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

@sophie-cluml sophie-cluml dismissed their stale review September 4, 2024 02:50

test code가 필요합니다.

@sophie-cluml
Copy link
Contributor

그리고 commit message의 giganto_config는 GraphQL API를 의미하신 것 같은데 맞을까요? 맞다면 gigantoConfig 로 변경 부탁드립니다.

src/settings.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@sophie-cluml sophie-cluml left a comment

Choose a reason for hiding this comment

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

테스트코드 코멘트만 확인 부탁드립니다. 나머지는 좋아보입니다.

src/graphql/status.rs Show resolved Hide resolved
Copy link
Contributor

@sophie-cluml sophie-cluml left a comment

Choose a reason for hiding this comment

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

LGTM

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 55 to 56
- Removed `unsafe` block in `write_run_tcpdump` while creating a temporary file.
- Remove migration code less than `0.15.3`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be in the section "Removed". What do you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

self.max_open_files
}

async fn max_mb_of_level_base(&self) -> u64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use StringNumber?

Copy link
Contributor

@sehkone sehkone Sep 6, 2024

Choose a reason for hiding this comment

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

Or, is this Okay because it is not directly returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@BLYKIM BLYKIM force-pushed the bly/command_line branch 3 times, most recently from 5100c39 to 87e34c9 Compare September 9, 2024 00:00
@kimhanbeom
Copy link
Contributor

@BLYKIM
Since the return type of GraphQL server is i32, it does not seem necessary to use StringNumber for types that do not lose precision, such as max_open_files, num_of_thread, and ack_transmission.

@sophie-cluml sophie-cluml merged commit 5811c47 into main Sep 10, 2024
10 checks passed
@sophie-cluml sophie-cluml deleted the bly/command_line branch September 10, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants