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

refactor(agent): various refactors #126

Merged
merged 7 commits into from
Jun 6, 2024
Merged

refactor(agent): various refactors #126

merged 7 commits into from
Jun 6, 2024

Conversation

Reisen
Copy link
Contributor

@Reisen Reisen commented Jun 5, 2024

This PR contains several refactors, each commit is self-contained and should be reviewed individually. You can leave comments directly on the commits. See the commit messages which might contain more information about a specific refactor.

Follow-up to:

Copy link
Collaborator

@ali-bahjati ali-bahjati left a comment

Choose a reason for hiding this comment

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

Very nice! I think one function call is removed by mistake but LGTM apart from that.

src/agent/state/keypairs.rs Show resolved Hide resolved
src/agent.rs Show resolved Hide resolved
src/agent/state/global.rs Show resolved Hide resolved
src/agent/state.rs Outdated Show resolved Hide resolved
@Reisen Reisen force-pushed the push-ymommvsnryks branch 2 times, most recently from c93bcf5 to a8ccdab Compare June 5, 2024 14:01
pub fn default_bind_address() -> SocketAddr {
"127.0.0.1:9001"
.parse()
.expect("INTERNAL: Could not build default remote keypair loader bind address")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you probably just copied this. IMO it's better to just do .unwrap for the statics that we know exactly what it is and it will never fail.

Comment on lines +209 to +210
/// NOTE(2023-03-22): Lifetime bounds are currently necessary
/// because of https://github.com/rust-lang/rust/issues/63033
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 this issue is now resolved, we might need to update rust tho. Maybe we can fix this in a future refactor PR.

Reisen added 5 commits June 6, 2024 09:42
The StateApi is left over from the initial Adapter, but all functionality is
for pricing/product accounts. This refactors that module and fixes the cyclic
dependency between it and GlobalStore.

The new logic performs updates within the Prices API (Which is where the state
relevant to subscriptions already was, so is the better place for it).

File rename left for a future commit to keep the diffs clean.
@Reisen Reisen force-pushed the push-ymommvsnryks branch from a8ccdab to 5a24855 Compare June 6, 2024 09:42
@Reisen Reisen merged commit 660b216 into main Jun 6, 2024
2 checks passed
@Reisen Reisen deleted the push-ymommvsnryks branch June 6, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants