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

Add features to monolithic_integration_test #362

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

divergentdave
Copy link
Collaborator

This moves most of monolithic_integration_test and its novel dependencies behind a new "test-util" feature flag, similar to the other
crates. With this, our release builds will no longer have the feature flags janus_core/test-util, janus_server/test-util, nor reqwest/default-tls set, due to crate feature flags being unified across the workspace. As a result, we can once again eliminate OpenSSL as a build-time dependency.

I also fixed a tangentially related inaccuracy in some comments that Tim pointed out.

This moves most of monolithic_integration_test and its novel
dependencies behind a new "test-util" feature flag, similar to the other
crates. With this, our release builds will no longer have the feature
flags janus_core/test-util, janus_server/test-util, nor
reqwest/default-tls set, due to crate feature flags being unified across
the workspace.
@divergentdave divergentdave requested a review from a team as a code owner August 4, 2022 15:09
@branlwyd
Copy link
Contributor

branlwyd commented Aug 4, 2022

While I don't mind doing this as a workaround, we really shouldn't have to introduce a test-util feature on a crate that is already test-only just to keep Cargo from bleeding its dependencies into other, unrelated crates. Every user of monolithic_integration_test is going to want to specify the test-util feature!

Could we instead factor this as janus & daphne features? (The former would enable use of the janus module, the latter the daphne module.) This would make usage of a feature more reasonable. And I suppose I exposed this functionality in the first place with the thought that others may want to build integration tests of their own against Janus; such a user would want to enable the janus feature but not the daphne feature, I suppose.

@@ -58,6 +58,6 @@ hex = { version = "*", features = ["serde"] }
janus_core = { path = ".", features = ["test-util"] }
# Enable `kube`'s `openssl-tls` feature (which takes precedence over the
# `rustls-tls` feature when creating a default client) to work around rustls's
# lack of support for IP addresses in certificates when connecting to a Kind
# cluster.
# lack of support for IP addresses in certificates when connecting to most
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 the specific condition is if the cluser is identified by IP, rather than DNS name -- IIUC rustls does not yet support IP-address-based certs.

Maybe "Kubernetes clusters identified by IP" or "local Kubernetes clusters" instead of "most Kubernetes clusters"? Both are more specific to the conditions that cause breakage.

@divergentdave divergentdave changed the title Add test-util feature to monolithic_integration_test Add features to monolithic_integration_test Aug 4, 2022
@divergentdave divergentdave merged commit c8dc255 into main Aug 5, 2022
@divergentdave divergentdave deleted the david/integration-test-feature branch August 5, 2022 14:32
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.

2 participants