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

ExistingNet -> NetWithFaucet #3067

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Dec 20, 2024

Motivation

As much as ExistingNet is better than the previous name, it's still not great.

Proposal

NetWithFaucet seems better and more descriptive of the network properties.

Test Plan

CI

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Dec 20, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ndr-ds ndr-ds force-pushed the 12-19-existingnet_-_netwithfaucet branch from b3bd9d2 to 8ca0682 Compare December 20, 2024 02:18
@ma2bd
Copy link
Contributor

ma2bd commented Dec 20, 2024

It makes more sense but it doesn't end up with "net" like the others. I still prefer the old name remote-net.

Copy link
Contributor Author

ndr-ds commented Dec 20, 2024

Maybe FaucetNet?

@@ -377,7 +377,7 @@ impl AmmApp {
#[cfg_attr(feature = "scylladb", test_case(LocalNetConfig::new_test(Database::ScyllaDb, Network::Grpc) ; "scylladb_grpc"))]
#[cfg_attr(feature = "dynamodb", test_case(LocalNetConfig::new_test(Database::DynamoDb, Network::Grpc) ; "aws_grpc"))]
#[cfg_attr(feature = "kubernetes", test_case(SharedLocalKubernetesNetTestingConfig::new(Network::Grpc, BuildArg::Build) ; "kubernetes_grpc"))]
#[cfg_attr(feature = "existing-net", test_case(ExistingNetTestingConfig::new(None) ; "existing_net_grpc"))]
#[cfg_attr(feature = "net-with-faucet", test_case(NetWithFaucetTestingConfig::new(None) ; "net_with_faucet_grpc"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

#[cfg_attr(feature = "faucet", test_case(FaucetNetTestingConfig::new(None) ; "faucet_grpc"))]

@@ -54,7 +54,7 @@ kubernetes = [
"dep:pathdiff",
"dep:fs_extra",
]
existing-net = []
net-with-faucet = []
Copy link
Contributor

Choose a reason for hiding this comment

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

faucet = []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't faucet-net be better here? Since this isn't general faucet functionality, but specific to the faucet net 🤔 I'll do faucet-net, but I can switch to faucet if you disagree

Copy link
Contributor

Choose a reason for hiding this comment

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

None of that matters because we want to change things in depth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, just left it as faucet-net for now then!

@ndr-ds ndr-ds force-pushed the 12-19-existingnet_-_netwithfaucet branch from 8ca0682 to e721d6e Compare December 20, 2024 14:45
@ndr-ds ndr-ds force-pushed the 12-19-existingnet_-_netwithfaucet branch from e721d6e to b18f542 Compare December 20, 2024 15:14
@ma2bd
Copy link
Contributor

ma2bd commented Dec 25, 2024

I still prefer the old name (until we refactor tests entirely, notably removing local-net) so I suggest we revert #2998 instead with PR #3073

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