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

Specify credential resources #583

Merged
merged 2 commits into from
Feb 5, 2025
Merged

Specify credential resources #583

merged 2 commits into from
Feb 5, 2025

Conversation

Westwooo
Copy link
Contributor

@Westwooo Westwooo commented Feb 5, 2025

Addresses: #576

let bucket = call.get_flag(engine_state, stack, "bucket")?;
let scopes_flag: Option<Value> = call.get_flag(engine_state, stack, "scopes")?;

let scopes = if let Some(scopes) = scopes_flag {
Copy link

Choose a reason for hiding this comment

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

NIT: I think if let Some() feels a bit redundant here given we're matching on scopes later anyway. Could we just match on scopes_flag instead? Maybe we could use guards for the bucket check. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that's a lot neater. And thanks for the link about 'guards' wasn't aware of them :)

Copy link

Choose a reason for hiding this comment

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

No problem, we could also flatten it by matching directly on e.g. Some(Value::String { val, .. }), but I think this is good too

@Westwooo Westwooo force-pushed the specify_credential_resources branch from dfb9509 to 2987825 Compare February 5, 2025 15:08
@Westwooo Westwooo merged commit f307be1 into main Feb 5, 2025
15 checks passed
@Westwooo Westwooo deleted the specify_credential_resources branch February 5, 2025 15:45
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