hubcio commented on code in PR #2190:
URL: https://github.com/apache/iggy/pull/2190#discussion_r2371708221
##########
core/ai/mcp/src/configs.rs:
##########
@@ -33,18 +42,10 @@ pub struct McpServerConfig {
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct IggyConfig {
pub address: String,
- pub username: Option<String>,
- pub password: Option<String>,
- pub token: Option<String>,
- pub consumer: Option<String>,
-}
-
-#[derive(Debug, Clone, Deserialize, Serialize)]
-pub struct HttpApiConfig {
- pub address: String,
- pub path: String,
- pub cors: Option<HttpCorsConfig>,
- pub tls: Option<HttpTlsConfig>,
+ pub username: String,
Review Comment:
I think it's not safe to store passwords in memory. Preferably, store hash
and compare with it upon some condition. Feel free to fix it later.
##########
core/ai/mcp/src/configs.rs:
##########
@@ -16,15 +16,24 @@
* under the License.
*/
+use std::fmt::Formatter;
+
Review Comment:
remove empty lines between use items
##########
core/common/src/configs/mod.rs:
##########
Review Comment:
I was thinking about adding one more check: if someone provides env variable
that has correct prefix, but is non-existent, e.g. `IGGY_TEST_ABC, should we
fail the server startup or just ignore it? currently, we will print to stdout
that some variable was overridden but we simply ignore it.
##########
core/connectors/sinks/postgres_sink/src/lib.rs:
##########
@@ -70,11 +70,16 @@ impl PostgresSink {
async fn connect(&mut self) -> Result<(), Error> {
let max_connections = self.config.max_connections.unwrap_or(10);
+ info!(
+ "Connecting to PostgreSQL database with max {} connections,
connection string: {}",
+ max_connections, self.config.connection_string
Review Comment:
connection strings often contains password, please don't print it as a whole
##########
core/integration/tests/connectors/postgres/mod.rs:
##########
@@ -0,0 +1,60 @@
+/* Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+use crate::connectors::{IggySetup, setup_runtime};
+use iggy::prelude::IggyClient;
+use std::collections::HashMap;
+use testcontainers_modules::{postgres, testcontainers::runners::AsyncRunner};
+
+mod postgres_sink;
+
+async fn setup() -> IggyClient {
+ let container = postgres::Postgres::default()
+ .start()
+ .await
+ .expect("Failed to start Postgres");
+ let host_port = container
+ .get_host_port_ipv4(5432)
+ .await
+ .expect("Failed to get Postgres port");
+
+ let mut envs = HashMap::new();
+ let iggy_setup = IggySetup::default();
+ let connection_string =
format!("postgres://postgres:postgres@localhost:{host_port}");
+ envs.insert(
+ "IGGY_CONNECTORS_SINKS_POSTGRES_CONFIG_CONNECTION_STRING".to_owned(),
+ connection_string,
+ );
+ envs.insert(
+ "IGGY_CONNECTORS_SINKS_POSTGRES_STREAMS_0_STREAM".to_owned(),
+ iggy_setup.stream.to_owned(),
+ );
+ envs.insert(
+ "IGGY_CONNECTORS_SINKS_POSTGRES_STREAMS_0_TOPICS_0".to_owned(),
+ iggy_setup.topic.to_owned(),
+ );
+ envs.insert(
+ "IGGY_CONNECTORS_SINKS_POSTGRES_STREAMS_0_SCHEMA".to_owned(),
+ "json".to_owned(),
+ );
+ let mut runtime = setup_runtime();
+ runtime
+ .init("postgres/postgres.toml", Some(envs), iggy_setup)
+ .await;
+ runtime.create_client().await
+}
Review Comment:
what about adding proper cleanup? :) either explicit via `impl Drop` or just
call somethink like `stop()` on `container()`
##########
core/connectors/runtime/example_config.toml:
##########
@@ -0,0 +1,178 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[http] # Optional HTTP API configuration
+enabled = true
+address = "127.0.0.1:8081"
+api_key = "" # Optional API key for authentication to be passed as `api-key`
header
+
+[http.cors] # Optional CORS configuration for HTTP API
+enabled = false
+allowed_methods = ["GET", "POST", "PUT", "DELETE"]
+allowed_origins = ["*"]
+allowed_headers = ["content-type"]
+exposed_headers = [""]
+allow_credentials = false
+allow_private_network = false
+
+[http.tls] # Optional TLS configuration for HTTP API
+enabled = false
+cert_file = "core/certs/iggy_cert.pem"
+key_file = "core/certs/iggy_key.pem"
+
+[iggy]
+address = "localhost:8090"
+username = "iggy"
+password = "iggy"
+token = "" # Personal Access Token (PAT) can be used instead of username and
password
+
+[state]
+path = "local_state"
+
+[sinks.stdout]
+enabled = true
+name = "Stdout sink"
+path = "target/release/libiggy_connector_stdout_sink"
+
+[[sinks.stdout.streams]]
+stream = "example_stream"
+topics = ["example_topic"]
+schema = "json"
+batch_length = 100
+poll_interval = "5ms"
+consumer_group = "stdout_sink_connector"
+
+[sinks.stdout.config]
+print_payload = false
+
+[sinks.stdout.transforms.add_fields]
+enabled = true
+
+[[sinks.stdout.transforms.add_fields.fields]]
+key = "message"
+value.static = "hello"
+
+[sources.random]
+enabled = true
+name = "Random source"
+path = "target/release/libiggy_connector_random_source"
+config_format = "json"
+
+[[sources.random.streams]]
+stream = "example_stream"
+topic = "example_topic"
+schema = "json"
+batch_length = 1000
+linger_time = "5ms"
+
+[sources.random.config]
+interval = "100ms"
+# max_count = 1000
Review Comment:
remove or uncomment
##########
core/common/src/lib.rs:
##########
@@ -39,6 +40,8 @@ pub use commands::system::get_cluster_metadata::*;
pub use commands::system::*;
pub use commands::topics::*;
pub use commands::users::*;
+// Configs
+pub use configs::*;
// Traits
Review Comment:
pls remove these useless comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
