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

use divviup-api-client in in-cluster tests #1697

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Aug 14, 2023

No description provided.

@jbr jbr requested a review from a team as a code owner August 14, 2023 18:57
@@ -35,6 +35,11 @@ serde_json = "1.0.103"
testcontainers = "0.14.0"
tokio.workspace = true
url = { version = "2.4.0", features = ["serde"] }
divviup-client = { git = "https://github.com/divviup/divviup-api", features = [
"admin",
] } # , tag = "0.0.19" }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eventually we'll want to use versions from crates.io

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should pin this to a particular commit with rev for the time being. A fresh working directory with this would fail now, since incompatible changes have been pushed to main.

min_batch_size: task.min_batch_size(),
max_batch_size: match task.query_type() {
QueryType::TimeInterval => None,
QueryType::FixedSize { max_batch_size, .. } => Some(*max_batch_size),
},
expiration: "3000-01-01T00:00:00Z".to_owned(),
expiration: None,
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 is the only known functional change here, and i don't know if it'll break anything. it was easier to type "None" than parse an OffsetDateTime

@jbr jbr force-pushed the use-divviup-api-client branch from 2d4fbad to 0257363 Compare August 14, 2023 19:09
@jbr jbr force-pushed the use-divviup-api-client branch from 9084632 to 5df767f Compare August 15, 2023 19:35
@jbr jbr changed the title wip: use divviup-api-client use divviup-api-client in in-cluster tests Aug 15, 2023
@jbr jbr force-pushed the use-divviup-api-client branch from 5df767f to 1d206b2 Compare August 15, 2023 19:43
@@ -35,6 +35,11 @@ serde_json = "1.0.103"
testcontainers = "0.14.0"
tokio.workspace = true
url = { version = "2.4.0", features = ["serde"] }
divviup-client = { git = "https://github.com/divviup/divviup-api", features = [
"admin",
] } # , tag = "0.0.19" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should pin this to a particular commit with rev for the time being. A fresh working directory with this would fail now, since incompatible changes have been pushed to main.

Cargo.lock Outdated Show resolved Hide resolved
@jbr jbr force-pushed the use-divviup-api-client branch 5 times, most recently from a85bb07 to 387438f Compare August 16, 2023 23:25
Copy link
Collaborator

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

I tested this prior to a recent rebase with some previously-discussed fixes and it worked against the cluster. At this point, the main branch is now using prio 0.14, which implements VDAF-06, and thus the integration test can no longer talk with the container images running in the cluster. I think we should fix the remaining compilation issues and land this, then backport the changes to the release/0.5 branch, which will still be run in the janus-ops CI, at least for the next few weeks.

integration_tests/tests/in_cluster.rs Outdated Show resolved Hide resolved
integration_tests/tests/in_cluster.rs Outdated Show resolved Hide resolved
integration_tests/tests/in_cluster.rs Outdated Show resolved Hide resolved
@jbr jbr force-pushed the use-divviup-api-client branch 5 times, most recently from 873d3d1 to e6eb815 Compare August 17, 2023 00:41
@tgeoghegan tgeoghegan force-pushed the use-divviup-api-client branch from e6eb815 to 41ce05b Compare September 18, 2023 19:35
@@ -15,6 +15,7 @@ in-cluster = ["dep:k8s-openapi", "dep:kube"]
anyhow.workspace = true
backoff = { version = "0.4", features = ["tokio"] }
base64.workspace = true
divviup-client = { git = "https://github.com/divviup/divviup-api", features = ["admin"], tag = "0.0.24" }
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work until we have divviup-api:0.0.25 that incorporates divviup/divviup-api#512

Copy link
Contributor Author

@jbr jbr Sep 18, 2023

Choose a reason for hiding this comment

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

Could always branch=main ¯\_(ツ)_/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, or pin a commit on main. But it's nicer to use a tag and releases aren't hard to do.

@tgeoghegan tgeoghegan force-pushed the use-divviup-api-client branch from 41ce05b to 298a288 Compare September 18, 2023 19:48
Re-enables the in-cluster Prio3Histogram test
@tgeoghegan tgeoghegan force-pushed the use-divviup-api-client branch from 298a288 to 923f7d6 Compare September 18, 2023 21:56
@tgeoghegan tgeoghegan merged commit 8fa5508 into divviup:main Sep 18, 2023
7 checks passed
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