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

[forge] Better error message when unauthorized #2137

Merged
merged 1 commit into from
Jul 22, 2022
Merged

[forge] Better error message when unauthorized #2137

merged 1 commit into from
Jul 22, 2022

Conversation

perryjrandall
Copy link
Contributor

@perryjrandall perryjrandall commented Jul 21, 2022

Print a better error message when you're not authorized. Additionally remove some overly complex logic for performing retries in favor of the more standard again crate.

I also added unittests and split up the function a bit to make it somewhat testable.

Test Plan:

Running unauthorized fails

$ cargo run -p forge-cli -- test k8s-swarm --namespace forge-banana --port-forward

2022-07-21T22:27:35.976025Z [main] INFO testsuite/forge/src/backend/k8s/mod.rs:53 Using forge namespace: forge-banana

running 1 tests
Starting Swarm with supported versions: ["devnet", "devnet"]
Failed to run tests:
Unauthorized, did you authorize with kubernetes? Try running `kubectl get current-context`
Error: Unauthorized, did you authorize with kubernetes? Try running `kubectl get current-context`

Then running authorized still works

$ cargo run -p forge-cli -- test k8s-swarm --namespace forge-banana --port-forward

====json-report-end===
2022-07-21T22:24:59.222911Z [main] INFO testsuite/forge/src/backend/k8s/cluster_helper.rs:279 Deleting namespace forge-banana: Some(NamespaceStatus { phase: Some("Terminating") })
2022-07-21T22:24:59.223309Z [main] INFO testsuite/forge/src/backend/k8s/cluster_helper.rs:390 aptos-node resources for Forge removed in namespace: forge-banana

test result: ok. 1 passed; 0 failed; 0 filtered out


This change is Reviewable

Print a better error message when you're not authorized. Additionally remove some overly complex logic for performing retries in favor of the more standard again crate.

I also added unittests and split up the function a bit to make it somewhat testable.

Test Plan:

Running unauthorized fails

```
$ cargo run -p forge-cli -- test k8s-swarm --namespace forge-banana --port-forward

2022-07-21T22:27:35.976025Z [main] INFO testsuite/forge/src/backend/k8s/mod.rs:53 Using forge namespace: forge-banana

running 1 tests
Starting Swarm with supported versions: ["devnet", "devnet"]
Failed to run tests:
Unauthorized, did you authorize with kubernetes? Try running `kubectl get current-context`
Error: Unauthorized, did you authorize with kubernetes? Try running `kubectl get current-context`
```

Then running authorized still works

```
$ cargo run -p forge-cli -- test k8s-swarm --namespace forge-banana --port-forward

====json-report-end===
2022-07-21T22:24:59.222911Z [main] INFO testsuite/forge/src/backend/k8s/cluster_helper.rs:279 Deleting namespace forge-banana: Some(NamespaceStatus { phase: Some("Terminating") })
2022-07-21T22:24:59.223309Z [main] INFO testsuite/forge/src/backend/k8s/cluster_helper.rs:390 aptos-node resources for Forge removed in namespace: forge-banana

test result: ok. 1 passed; 0 failed; 0 filtered out

```
@perryjrandall perryjrandall added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 21, 2022
@perryjrandall perryjrandall enabled auto-merge (rebase) July 21, 2022 23:39
@perryjrandall perryjrandall merged commit f950804 into main Jul 22, 2022
@perryjrandall perryjrandall deleted the forgery branch July 22, 2022 00:06
@github-actions
Copy link
Contributor

✅ Forge test success

Forge is land-blocking

all up : 6527 TPS, 2581 ms latency, 3600 ms p99 latency,no expired txns

spec: None,
status: None,
};
if let Err(KubeError::Api(api_err)) = namespace_creator
Copy link
Contributor

Choose a reason for hiding this comment

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

You only include api_err itself in one of the error arms, it might be good to do it in all of the arms for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants