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

feat: start logging diff comparing with delta #666

Merged
merged 19 commits into from
Jan 16, 2025
Merged

feat: start logging diff comparing with delta #666

merged 19 commits into from
Jan 16, 2025

Conversation

sjaanus
Copy link
Collaborator

@sjaanus sjaanus commented Jan 15, 2025

Now when client/features is called, it will also call delta API and verify that payload matches.

This is behind feature flag.

Copy link

github-actions bot commented Jan 15, 2025

Dependency Review

The following issues were found:
  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ⚠️ 1 package(s) with unknown licenses.
See the Details below.

License Issues

server/Cargo.toml

PackageVersionLicenseIssue Type
unleash-yggdrasil>= 0.14.6, < 0.15.0NullUnknown License
Denied Licenses: GPL-1.0, GPL-2.0, GPL-3.0, LGPL-2.1, LGPL-3.0, AGPL-3.0

OpenSSF Scorecard

PackageVersionScoreDetails
cargo/difflib 0.4.0 🟢 3.3
Details
CheckScoreReason
Dangerous-Workflow⚠️ -1no workflows found
Token-Permissions⚠️ -1No tokens found
Code-Review⚠️ 2Found 4/18 approved changesets -- score normalized to 2
Packaging⚠️ -1packaging workflow not detected
Binary-Artifacts🟢 10no binaries found in the repo
Pinned-Dependencies⚠️ -1no dependencies found
Maintained⚠️ 00 commit(s) and 0 issue activity found in the last 90 days -- score normalized to 0
CII-Best-Practices⚠️ 0no effort to earn an OpenSSF best practices badge detected
Security-Policy⚠️ 0security policy file not detected
Fuzzing⚠️ 0project is not fuzzed
Vulnerabilities🟢 100 existing vulnerabilities detected
License🟢 10license file detected
Signed-Releases⚠️ -1no releases found
Branch-Protection⚠️ 0branch protection not enabled on development/release branches
SAST⚠️ 0SAST tool is not run on all commits -- score normalized to 0
cargo/json-structural-diff 0.2.0 UnknownUnknown
cargo/unleash-types 0.15.4 UnknownUnknown
cargo/unleash-yggdrasil 0.14.6 UnknownUnknown
cargo/json-structural-diff >= 0.2.0, < 0.3.0 UnknownUnknown
cargo/unleash-types >= 0.15.4, < 0.16.0 UnknownUnknown
cargo/unleash-yggdrasil >= 0.14.6, < 0.15.0 UnknownUnknown

Scanned Files

  • Cargo.lock
  • server/Cargo.toml

@sjaanus sjaanus requested review from chriswk and sighphyre January 15, 2025 13:11
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Sure

server/src/cli.rs Outdated Show resolved Hide resolved
@@ -470,7 +509,7 @@ impl FeatureRefresher {
.unleash_client
.get_client_features(ClientFeaturesRequest {
api_key: refresh.token.token.clone(),
etag: refresh.etag,
etag: refresh.etag.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

why did it suddenly need a clone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was due to

 --> server/src/http/refresher/feature_refresher.rs:526:50
    |
512 |                 etag: refresh.etag,
    |                       ------------ value partially moved here
...
526 |                         self.compare_delta_cache(&refresh).await;
    |                                                  ^^^^^^^^ value borrowed here after partial move

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Some nits, but I'm 👍 here. Good job

@sjaanus sjaanus merged commit 9abcfee into main Jan 16, 2025
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants