Relayer url and api_key moved to the partner struct, refactoring #286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Terraform Dev EnvironmentTerraform Format and Style 🖌
|
Terraform Feature Environment (dev-286)Terraform Initialization ⚙️
|
itegulov
approved these changes
Sep 13, 2023
There was a problem hiding this comment.
LGTM, just a few nitpicks!
I would test this setup using Danny's frontend just to make sure the partner list gets populated correctly via terraform.
Also, maybe we should invest in PVT tests at this point. We could start by simply running a subset of the existing integration tests once terraform env is up.
integration-tests/src/containers.rs
Outdated
| "audience": firebase_audience_id, | ||
| }, | ||
| "relayer": { | ||
| "url": "http://relayer:3000", // not used |
There was a problem hiding this comment.
hmm, does it make sense that signer node accepts relayer url that is not actually used anywhere?
There was a problem hiding this comment.
Good point. Refactored. Now Leader Node accepts list of partners and Signing node accepts list of OIDC Providers.
| partner.relayer.clone(), | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
We are still using this horrible hack huh 😬
There was a problem hiding this comment.
Yes... talked about this with Anthony, he did not wanted to allowlist our account creaor :)
volovyks
commented
Sep 13, 2023
| partner.relayer.clone(), | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Yes... talked about this with Anthony, he did not wanted to allowlist our account creaor :)
| } | ||
| } | ||
| _ => return Err(anyhow::anyhow!("Invalid combination of data and path")), |
There was a problem hiding this comment.
this last _ case won't be necessary since we handled all the cases. best to just get rid of it
Suggested change
| _ => return Err(anyhow::anyhow!("Invalid combination of data and path")), |
There was a problem hiding this comment.
Some Some is not handled.
| { | ||
| Some(partner) => Ok(partner), | ||
| None => Err(anyhow::anyhow!( | ||
| "Failed to find relayer for given partner. Issuer: {}, Audience: {}", |
There was a problem hiding this comment.
I don't think we should be returning this specific error message here. Instead we should have it in the calling code, because it depends on which context we're querying the partner list. So we can just return an Option<FastAuthPartner> for now
There was a problem hiding this comment.
It was like this in my initial design, but looked clunky: ebb3e1f
Co-authored-by: Phuong Nguyen <[email protected]>
Terraform Feature Environment Destroy (dev-286)Terraform Initialization ⚙️
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.