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

Read Zulip config from env vars #1872

Merged
merged 1 commit into from
Jan 4, 2025

Conversation

apiraino
Copy link
Contributor

@apiraino apiraino commented Dec 27, 2024

Sometimes we want to test the triagebot against a test Zulip instance (f.e. #1853) but currently the Zulip config is hardcoded. This patch allows settings env vars to use a custom Zulip instance. If these env vars are not set, the triagebot will work as usual and connect to the production Zulip instance.

I've tested this with a binary that I didn't include in the repository, it simply uses the public api is zulip.rs:

code

use triagebot::github::GithubClient;
use triagebot::zulip;

#[tokio::main(flavor = "current_thread")]
async fn main() -> anyhow::Result<()> {
    let gh = GithubClient::new_from_env();

    let zulip_topic_name = "New topic";
    let message = "This is a message";

    let zulip_req = crate::zulip::MessageApiRequest {
        recipient: crate::zulip::Recipient::Stream {
            id: 123456, // #ChannelName
            topic: &zulip_topic_name,
        },
        content: &message,
    };

    // .send() reads env vars ZULIP_URL, ZULIP_API_TOKEN, ZULIP_BOT_EMAIL
    let response = zulip_req.send(&gh.raw()).await?;
    println!("RESP: {:?}", response.text().await);

    Ok(())
}

maybe r? @ehuss (or anyone who has time for a review)

@@ -71,8 +75,6 @@ struct Response {
content: String,
}

pub const BOT_EMAIL: &str = "[email protected]";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to also set this global but then it needs to be cloned multiple times, so unsure what's the best solution

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I see the concern about needing to be cloned. If you do it in a LazyLock like you did the URL, then you can do &*ZULIP_BOT_EMAIL to get a reference. The basic_auth method does not need an owned value.

An alternative would be to place all these environment values inside a structure stored in the Context, and just pass that around as an argument (or pass in the whole Context).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for a second opinion! I've amended the code with just adding another LazyLock to keep changes as contained here as possible.

@apiraino apiraino force-pushed the move-zulip-config-to-env-vars branch from c096e57 to ec08941 Compare January 2, 2025 14:15
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Thanks!

@ehuss ehuss merged commit cde54de into rust-lang:master Jan 4, 2025
2 checks passed
@apiraino apiraino deleted the move-zulip-config-to-env-vars branch January 4, 2025 23:00
@apiraino
Copy link
Contributor Author

apiraino commented Jan 4, 2025

thanks @ehuss !

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