You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "atharvalade (via GitHub)" <gi...@apache.org> on 2026/03/20 18:14:06 UTC
[PR] Add context create/delete CLI commands to complete login/logout workflow [iggy]
atharvalade opened a new pull request, #2998:
URL: https://github.com/apache/iggy/pull/2998
## Which issue does this PR close?
Closes #595
## Rationale
The login/logout commands and context infrastructure (list, use) already existed, but users had to manually edit `~/.iggy/contexts.toml` to create or remove contexts. This PR adds the missing `context create` and `context delete` subcommands so the entire credential workflow can be managed entirely through the CLI, similar to `docker login`/`docker logout` + `docker context`.
## What changed?
Users could not create or delete contexts via the CLI — they had to manually edit TOML files. This made the docker-login-like workflow incomplete.
Added `iggy context create <name> [--transport, --tcp-server-address, --username, ...]` and `iggy context delete <name>` commands with proper validation (duplicate prevention, cannot delete `default`, active context reset on deletion), along with integration tests covering success paths, edge cases, and help text.
## Local Execution
- Passed
- Pre-commit hooks ran
## AI Usage
1. Opus 4.6
2. Minimal AI used
3. Verified by building, running all commands locally with edge cases, and running cargo fmt/clippy/license checks
4. Yes
<img width="647" height="216" alt="Screenshot 2026-03-20 at 2 08 46 PM" src="https://github.com/user-attachments/assets/a5e298b3-f227-4227-aa65-d2e1d3d8f079" />
<img width="758" height="698" alt="Screenshot 2026-03-20 at 2 08 53 PM" src="https://github.com/user-attachments/assets/1d6ca7f2-a66f-40ae-9a1d-ea6ada134c92" />
<img width="702" height="278" alt="Screenshot 2026-03-20 at 2 09 01 PM" src="https://github.com/user-attachments/assets/4bf1a14f-fa8e-4ab9-a645-36495fbb9cd8" />
<img width="812" height="80" alt="Screenshot 2026-03-20 at 2 09 16 PM" src="https://github.com/user-attachments/assets/188f2783-f762-4914-8b65-690e973e0e5c" />
<img width="822" height="77" alt="Screenshot 2026-03-20 at 2 09 22 PM" src="https://github.com/user-attachments/assets/e87c80f3-4c20-4332-acbe-a01a1b1ccc47" />
<img width="739" height="164" alt="Screenshot 2026-03-20 at 2 09 29 PM" src="https://github.com/user-attachments/assets/d50aa222-b52e-40d5-88f8-9c58b838b664" />
<img width="802" height="173" alt="Screenshot 2026-03-20 at 2 09 49 PM" src="https://github.com/user-attachments/assets/cd45fbcf-41e2-4ae4-8b47-31612940a775" />
<img width="811" height="186" alt="Screenshot 2026-03-20 at 2 09 56 PM" src="https://github.com/user-attachments/assets/db72994c-0bee-41e5-b822-2effc06464f8" />
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996286692
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
Review Comment:
added `validate_context_name` that rejects empty, whitespace-only, and non-alphanumeric/hyphen/underscore names, plus an explicit if `name == ``DEFAULT_CONTEXT_NAME` guard in create_context.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996284523
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
+ self.get_context_state().await?;
+ let cs = self.context_state.as_ref().unwrap();
+
+ if cs.contexts.contains_key(name) {
+ bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}")
+ }
+
+ let mut new_contexts = cs.contexts.clone();
+ new_contexts.insert(name.to_string(), config);
+
+ self.context_rw.ensure_iggy_home_exists()?;
+ self.context_rw
+ .write_contexts(new_contexts.clone())
Review Comment:
Thanks for catching this -- added `set_owner_only_permissions` that sets 0600 on `contexts.toml` after every write (unix only, no-op on Windows).
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "BartoszCiesla (via GitHub)" <gi...@apache.org>.
BartoszCiesla commented on PR #2998:
URL: https://github.com/apache/iggy/pull/2998#issuecomment-4100479191
@hubcio it would be good to cross check if options defined for `context create` match corresponding options in basic commands - from what I've checked this pull request contains only long versions and AFAIR at least some of the options had short versions (`--password` and `-p`). It would be best to reuse the descriptions - either by copying the existing `clap` content or following / re-using existing structs (if possible). Keeping the information consistent will make things much easier for the user. But that's just my view ;-)
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on PR #2998:
URL: https://github.com/apache/iggy/pull/2998#issuecomment-4129030735
Great thought, thanks for the review @BartoszCiesla. I have updated the credential options to include short flags `(-u, -p, -t, -n)` matching the existing CLI options in `CliOptions`, and aligned all descriptions with the existing `CliOptions` and `ArgsOptional` structs so the user experience stays consistent.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #2998:
URL: https://github.com/apache/iggy/pull/2998#issuecomment-4100090089
@BartoszCiesla perhaps you want to review? :)
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996292741
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -258,6 +321,18 @@ impl ContextReaderWriter {
Ok(())
}
+ pub fn ensure_iggy_home_exists(&self) -> Result<()> {
+ if let Some(ref iggy_home) = self.iggy_home
+ && !iggy_home.exists()
+ {
+ std::fs::create_dir_all(iggy_home).context(format!(
Review Comment:
replaced `std::fs::create_dir_all` and `iggy_home.exists()` with t`okio::fs::create_dir_all` and `tokio::fs::try_exists` to stay consistent with the rest of the file.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996301934
##########
core/cli/src/commands/binary_context/common.rs:
##########
Review Comment:
switched `ContextsConfigMap` from `HashMap` to `BTreeMap` so context sections in `contexts.toml` are always written in alphabetical order
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2998:
URL: https://github.com/apache/iggy/pull/2998#issuecomment-4129093676
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/2998?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `36.36364%` with `70 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.22%. Comparing base ([`601e597`](https://app.codecov.io/gh/apache/iggy/commit/601e59776ad5aaaf12218d9c72d40649bf770270?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`49ec092`](https://app.codecov.io/gh/apache/iggy/commit/49ec0925c26b5aac2361d7a7da68caba83e256bc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/2998?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [core/cli/src/commands/binary\_context/common.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_context%2Fcommon.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9jb250ZXh0L2NvbW1vbi5ycw==) | 41.07% | [30 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../cli/src/commands/binary\_context/create\_context.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_context%2Fcreate_context.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9jb250ZXh0L2NyZWF0ZV9jb250ZXh0LnJz) | 0.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/cli/src/args/context.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fargs%2Fcontext.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2FyZ3MvY29udGV4dC5ycw==) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/cli/src/main.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fmain.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL21haW4ucnM=) | 40.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your patch check has failed because the patch coverage (36.36%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2998 +/- ##
=============================================
- Coverage 72.18% 61.22% -10.96%
Complexity 930 930
=============================================
Files 1122 1125 +3
Lines 93502 93612 +110
Branches 70851 70961 +110
=============================================
- Hits 67494 57317 -10177
- Misses 23441 33784 +10343
+ Partials 2567 2511 -56
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.30% <36.36%> (-14.65%)` | :arrow_down: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.08% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.64% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.43% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.40% <ø> (ø)` | |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/2998/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.68% <ø> (ø)` | |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/2998?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../cli/src/commands/binary\_context/delete\_context.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_context%2Fdelete_context.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9jb250ZXh0L2RlbGV0ZV9jb250ZXh0LnJz) | `100.00% <100.00%> (ø)` | |
| [core/cli/src/main.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fmain.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL21haW4ucnM=) | `51.85% <40.00%> (-40.23%)` | :arrow_down: |
| [core/cli/src/args/context.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fargs%2Fcontext.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2FyZ3MvY29udGV4dC5ycw==) | `0.00% <0.00%> (ø)` | |
| [.../cli/src/commands/binary\_context/create\_context.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_context%2Fcreate_context.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9jb250ZXh0L2NyZWF0ZV9jb250ZXh0LnJz) | `0.00% <0.00%> (ø)` | |
| [core/cli/src/commands/binary\_context/common.rs](https://app.codecov.io/gh/apache/iggy/pull/2998?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_context%2Fcommon.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9jb250ZXh0L2NvbW1vbi5ycw==) | `66.01% <41.07%> (-17.99%)` | :arrow_down: |
... and [277 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/2998/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on PR #2998:
URL: https://github.com/apache/iggy/pull/2998#issuecomment-4129089704
I think #3000 and #3030 are good follow-ups/pre-existing bug. Would love to hear any feedback @hubcio @BartoszCiesla
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996290458
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
+ self.get_context_state().await?;
+ let cs = self.context_state.as_ref().unwrap();
+
+ if cs.contexts.contains_key(name) {
+ bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}")
+ }
+
+ let mut new_contexts = cs.contexts.clone();
+ new_contexts.insert(name.to_string(), config);
+
+ self.context_rw.ensure_iggy_home_exists()?;
+ self.context_rw
+ .write_contexts(new_contexts.clone())
+ .await
+ .context(format!("failed writing contexts after creating '{name}'"))?;
+
+ self.context_state.replace(ContextState {
+ active_context: cs.active_context.clone(),
+ contexts: new_contexts,
+ });
+
+ Ok(())
+ }
+
+ pub async fn delete_context(&mut self, name: &str) -> Result<()> {
+ if name == DEFAULT_CONTEXT_NAME {
+ bail!("cannot delete the '{DEFAULT_CONTEXT_NAME}' context")
+ }
+
+ self.get_context_state().await?;
+ let cs = self.context_state.take().unwrap();
Review Comment:
switched `delete_context` to use `.as_ref().unwrap()` with `.clone()` to match `create_context` and avoid leaving `context_state` as `None` if a later operation bails
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz merged PR #2998:
URL: https://github.com/apache/iggy/pull/2998
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2994228734
##########
core/cli/src/commands/binary_context/delete_context.rs:
##########
@@ -0,0 +1,61 @@
+/* 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 async_trait::async_trait;
+use tracing::{Level, event};
+
+use crate::commands::cli_command::{CliCommand, PRINT_TARGET};
+use iggy_common::Client;
+
+use super::common::ContextManager;
+
+pub struct DeleteContextCmd {
+ context_name: String,
+}
+
+impl DeleteContextCmd {
+ pub fn new(context_name: String) -> Self {
+ Self { context_name }
+ }
+}
+
+#[async_trait]
+impl CliCommand for DeleteContextCmd {
+ fn explain(&self) -> String {
+ let context_name = &self.context_name;
+ format!("delete context {context_name}")
+ }
+
+ fn login_required(&self) -> bool {
+ false
+ }
+
+ fn connection_required(&self) -> bool {
+ false
+ }
+
+ async fn execute_cmd(&mut self, _client: &dyn Client) -> anyhow::Result<(), anyhow::Error> {
+ let mut context_mgr = ContextManager::default();
+
+ context_mgr.delete_context(&self.context_name).await?;
+
+ event!(target: PRINT_TARGET, Level::INFO, "context '{}' deleted successfully", self.context_name);
+
+ Ok(())
+ }
+}
Review Comment:
nit: `create_context.rs` has a `#[cfg(test)]` module with tests for `explain()`, `login_required()`, and `connection_required()`. `delete_context.rs` is missing the same for consistency.
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
Review Comment:
no validation on `name` - empty strings, whitespace-only, and control characters are all accepted. an empty string creates a `[""]
` TOML section. the toml crate handles quoting for most special chars and fails cleanly for truly bad ones (e.g. newlines in table keys), so this isn't a corruption risk, but it produces confusing config entries.
a basic check like `name.trim().is_empty()` plus maybe an alphanumeric+hyphen+underscore filter would be a good guard.
also: `create_context("default", ...)` is implicitly blocked because `get_context_state` always seeds a "default" entry, so `contains_key` catches it. but an explicit `if name == DEFAULT_CONTEXT_NAME { bail!(...) }` guard (like `delete_context` has) would be more resilient to future changes.
##########
core/cli/src/args/context.rs:
##########
@@ -50,3 +75,134 @@ pub(crate) struct ContextUseArgs {
#[arg(value_parser = clap::value_parser!(String))]
pub(crate) context_name: String,
}
+
+#[derive(Debug, Clone, Args)]
+pub(crate) struct ContextCreateArgs {
+ /// Name of the context to create
+ #[arg(value_parser = clap::value_parser!(String))]
+ pub(crate) context_name: String,
+
+ /// The transport to use
+ ///
+ /// Valid values are "quic", "http", "tcp" and "ws".
+ #[clap(verbatim_doc_comment, long)]
+ pub(crate) transport: Option<String>,
Review Comment:
`transport` is a free-form `Option<String>` with no parse-time validation. `--transport foobar` gets silently written to `contexts.toml` and only fails later at connection time with an unrelated error.
a `TransportProtocol` enum already exists in the codebase (`core/common/src/types/configuration/transport.rs`) with strum-based `FromStr`. you could validate here with `clap::value_parser!` or at minimum call `TransportProtocol::from_str` in `create_context` before writing.
this follows the pre-existing `ArgsOptional` pattern, so not a regression - but `context create` is a new user-facing entry point where fail-fast validation matters more.
also heads up: there's a pre-existing mismatch between `TransportProtocol`'s strum serialization (`"ws"`) and `Args::get_server_address` which matches against `"websocket"`. a user creating a context with `--transport ws` would hit this.
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -258,6 +321,18 @@ impl ContextReaderWriter {
Ok(())
}
+ pub fn ensure_iggy_home_exists(&self) -> Result<()> {
+ if let Some(ref iggy_home) = self.iggy_home
+ && !iggy_home.exists()
+ {
+ std::fs::create_dir_all(iggy_home).context(format!(
Review Comment:
`std::fs::create_dir_all` is a blocking call inside an async context. also `iggy_home.exists()` in the guard is blocking. `tokio::fs::create_dir_all` would be the async-consistent replacement. not a practical issue for a CLI, but inconsistent with the rest of this file which uses `tokio::fs` everywhere.
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
+ self.get_context_state().await?;
+ let cs = self.context_state.as_ref().unwrap();
+
+ if cs.contexts.contains_key(name) {
+ bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}")
+ }
+
+ let mut new_contexts = cs.contexts.clone();
+ new_contexts.insert(name.to_string(), config);
+
+ self.context_rw.ensure_iggy_home_exists()?;
+ self.context_rw
+ .write_contexts(new_contexts.clone())
Review Comment:
this is the first production code path that writes credentials (`--password`, `--token`) to `contexts.toml`. `tokio::fs::write` creates files with default permissions (typically `0644` - world-readable). should set `0600` on the file after writing, or use a file-creation API that sets restrictive permissions. same applies to the `write_contexts` call in `delete_context` at line 180.
##########
core/cli/src/commands/binary_context/common.rs:
##########
Review Comment:
`#[serde(flatten)]` on `ArgsOptional` means unknown fields are silently dropped during deserialization. since `create_context` and `delete_context` do a read-all/modify-one/write-all cycle on `contexts.toml`, running an older CLI binary against a config written by a newer version will strip any new fields from ALL context entries - not just the one being created/deleted. consider adding `#[serde(flatten)] pub extra: HashMap<String, toml::Value>` to `ContextConfig` to preserve unknown keys through round-trips.
##########
core/cli/src/commands/binary_context/common.rs:
##########
@@ -126,6 +126,69 @@ impl ContextManager {
Ok(context_state.contexts.clone())
}
+ pub async fn create_context(&mut self, name: &str, config: ContextConfig) -> Result<()> {
+ self.get_context_state().await?;
+ let cs = self.context_state.as_ref().unwrap();
+
+ if cs.contexts.contains_key(name) {
+ bail!("context '{name}' already exists in {CONTEXTS_FILE_NAME}")
+ }
+
+ let mut new_contexts = cs.contexts.clone();
+ new_contexts.insert(name.to_string(), config);
+
+ self.context_rw.ensure_iggy_home_exists()?;
+ self.context_rw
+ .write_contexts(new_contexts.clone())
+ .await
+ .context(format!("failed writing contexts after creating '{name}'"))?;
+
+ self.context_state.replace(ContextState {
+ active_context: cs.active_context.clone(),
+ contexts: new_contexts,
+ });
+
+ Ok(())
+ }
+
+ pub async fn delete_context(&mut self, name: &str) -> Result<()> {
+ if name == DEFAULT_CONTEXT_NAME {
+ bail!("cannot delete the '{DEFAULT_CONTEXT_NAME}' context")
+ }
+
+ self.get_context_state().await?;
+ let cs = self.context_state.take().unwrap();
Review Comment:
minor: `delete_context` uses `.take().unwrap()` here while `create_context` uses `.as_ref().unwrap()` at line 131. the `.take()` pattern means `context_state` is `None` if any subsequent operation bails (lines 163, 171, 180). `create_context`'s `.as_ref()` approach is safer since it doesn't consume the state.
in practice this doesn't matter - every `execute_cmd` creates a fresh `ContextManager` and discards it, so no caller ever makes a second call on the same instance. but for API consistency and robustness, matching `create_context`'s pattern would be cleaner.
(the pre-existing `set_active_context_key` at line 100 has the same `.take()` pattern.)
##########
core/integration/tests/cli/context/common.rs:
##########
@@ -64,6 +64,10 @@ impl TestIggyContext {
self.context_manager.read_active_context().await.unwrap()
Review Comment:
`read_active_context` returns the raw `read_to_string` result without `.trim()`. if a user (or text editor) adds a trailing newline to `.active_context`, the key won't match any context and `get_context_state` bails with a confusing "missing" error. adding `.map(|s| s.trim().to_string())` is cheap insurance.
##########
core/cli/src/commands/binary_context/common.rs:
##########
Review Comment:
`ContextsConfigMap` is `HashMap<String, ContextConfig>`. every write may reorder sections in `contexts.toml`. `BTreeMap` would give stable, alphabetically sorted output. pre-existing, but worth a follow-up.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996296286
##########
core/cli/src/commands/binary_context/delete_context.rs:
##########
@@ -0,0 +1,61 @@
+/* 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 async_trait::async_trait;
+use tracing::{Level, event};
+
+use crate::commands::cli_command::{CliCommand, PRINT_TARGET};
+use iggy_common::Client;
+
+use super::common::ContextManager;
+
+pub struct DeleteContextCmd {
+ context_name: String,
+}
+
+impl DeleteContextCmd {
+ pub fn new(context_name: String) -> Self {
+ Self { context_name }
+ }
+}
+
+#[async_trait]
+impl CliCommand for DeleteContextCmd {
+ fn explain(&self) -> String {
+ let context_name = &self.context_name;
+ format!("delete context {context_name}")
+ }
+
+ fn login_required(&self) -> bool {
+ false
+ }
+
+ fn connection_required(&self) -> bool {
+ false
+ }
+
+ async fn execute_cmd(&mut self, _client: &dyn Client) -> anyhow::Result<(), anyhow::Error> {
+ let mut context_mgr = ContextManager::default();
+
+ context_mgr.delete_context(&self.context_name).await?;
+
+ event!(target: PRINT_TARGET, Level::INFO, "context '{}' deleted successfully", self.context_name);
+
+ Ok(())
+ }
+}
Review Comment:
added a matching `#[cfg(test)] `module with tests for `explain(),` `login_required()`, and `connection_required()`
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996288391
##########
core/cli/src/args/context.rs:
##########
@@ -50,3 +75,134 @@ pub(crate) struct ContextUseArgs {
#[arg(value_parser = clap::value_parser!(String))]
pub(crate) context_name: String,
}
+
+#[derive(Debug, Clone, Args)]
+pub(crate) struct ContextCreateArgs {
+ /// Name of the context to create
+ #[arg(value_parser = clap::value_parser!(String))]
+ pub(crate) context_name: String,
+
+ /// The transport to use
+ ///
+ /// Valid values are "quic", "http", "tcp" and "ws".
+ #[clap(verbatim_doc_comment, long)]
+ pub(crate) transport: Option<String>,
Review Comment:
added `validate_transport` in `create_context.rs` that calls `TransportProtocol::from_str `before writing, so invalid values like --transport foobar now fail immediately with a clear message listing valid options.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996300403
##########
core/integration/tests/cli/context/common.rs:
##########
@@ -64,6 +64,10 @@ impl TestIggyContext {
self.context_manager.read_active_context().await.unwrap()
Review Comment:
added `.map(|s| s.trim().to_string())` to `read_active_context` so a trailing newline from a text editor won't cause a confusing "missing" error
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cli): Add context create/delete CLI commands to complete login/logout workflow [iggy]
Posted by "atharvalade (via GitHub)" <gi...@apache.org>.
atharvalade commented on code in PR #2998:
URL: https://github.com/apache/iggy/pull/2998#discussion_r2996298614
##########
core/cli/src/commands/binary_context/common.rs:
##########
Review Comment:
added pub extra: `BTreeMap<String, toml::Value> `with `#[serde(flatten)]` to `ContextConfig` so unknown keys from newer CLI versions survive the read-modify-write cycle.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org