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

Migrate to humantime (phase 1) #2189

Closed
wants to merge 2 commits into from
Closed

Conversation

squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Dec 19, 2023

Description

Closes phase 1 from the #2085. Since DEX solvers are about to migrate to a separate repo, it's a good time to change the config format. It will be much more difficult later.

Changes

Support both plain and humantime formats to migrate our Pulumi config gradually.

How to test

e2e + staging

@squadgazzz squadgazzz force-pushed the migrate-to-humantime-phase1 branch from 62a74b9 to 6255e69 Compare December 19, 2023 16:36
@squadgazzz squadgazzz marked this pull request as ready for review December 19, 2023 16:39
@squadgazzz squadgazzz requested a review from a team as a code owner December 19, 2023 16:39
@squadgazzz squadgazzz changed the title Migrate to humantime (phase 1) Migrate to humantime (phase 1) Dec 19, 2023
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Looks good, just a small confusion around the continuation of "from_days/from_seconds"

Comment on lines +379 to +382
humantime::parse_duration(s).or_else(|_| {
s.parse::<f64>()
.map(|days| Duration::from_secs_f64(days * 86_400.0))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not remove this method in favor of humantime::parse_duration altogether? I think it's weird to have a method duration_from_day that you can call with "10m"

Or is this phase2 after the pulumi config update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is this phase2 after the pulumi config update?

Yes, this method will be either removed or renamed since s.parse::<f64>().map(|days| Duration::from_secs_f64(days * 86_400.0)) part will be gone for sure.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

I think I would not switch over in multiple phases.
The infra PR that changes from the old config to the new one can be prepared before hand and verified locally. And when this PR gets merged it can quickly be applied.
When we apply the changes together in prod next week we'll have no trouble.
So I think the only upside to splitting this change into multiple phases is avoiding a tiny downtime on staging which would be totally okay. It's a testing environment which is allowed to break from time to time.

@squadgazzz
Copy link
Contributor Author

squadgazzz commented Dec 20, 2023

When we apply the changes together in prod next week we'll have no trouble.

I was worried about that exactly. Why there won't be downtime as we would have in staging? infra and services are deployed at the same time in prod?

The infra PR that changes from the old config to the new one can be prepared before hand and verified locally.

Not sure how to test it locally before merging this PR. Applying the new config will fail until this PR is not on staging.

@MartinquaXD
Copy link
Contributor

Why there won't be downtime as we would have in staging? infra and services are deployed at the same time in prod?

Exactly.

Not sure how to test it locally before merging this PR.

You could probably log the content of the files pulumi would create if you were to apply the change and try to run your local system with those to see if things break. But that might be a bit involved if you want to make sure everything works.

Alternatively it should be possible to build a docker container from the PR branch and use that when preparing your infra change. Then you can play around with both changes together on sepolia for example until they work together.

IIRC in order to make CI build a container from your branch it should have a tag starting with v (see CI).

@squadgazzz squadgazzz marked this pull request as draft December 20, 2023 11:44
@squadgazzz
Copy link
Contributor Author

#2194

@squadgazzz squadgazzz closed this Dec 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants