Improve login command ux #859

rmn-boiko · 2024-09-26T13:19:54Z

Description

Closes: #450

Checklist before requesting a review

s373r

Add comments & questions

src/adapter/http/src/data/account_handler.rs

src/adapter/http/src/data/dataset_info_handler.rs

src/adapter/http/src/data/workspace_info_handler.rs

src/infra/core/src/push_service_impl.rs

src/infra/core/src/remote_alias_resolver_impl.rs

sergiimk

Did a quick pass. Structurally things are looking very good.

I'd like to dig in a bit into DatasetRefRemote and TransferDatasetRef types and see if they can be simplified. Left some ideas.

src/adapter/http/src/data/account_handler.rs

src/adapter/http/src/data/query_handler.rs

src/adapter/http/src/data/mod.rs

src/adapter/http/src/data/workspace_info_handler.rs

src/adapter/http/src/data/router.rs

src/app/cli/src/cli_value_parser.rs

src/domain/core/src/services/push_service.rs

src/domain/core/src/services/remote_aliases_registry.rs

src/adapter/http/src/data/account_handler.rs

src/adapter/http/src/data/dataset_info_handler.rs

src/adapter/http/src/data/workspace_info_handler.rs

src/infra/core/src/remote_alias_resolver_impl.rs

sergiimk · 2024-10-11T16:43:45Z

src/domain/core/src/services/remote_aliases_registry.rs

+}
+
+#[derive(Debug, Clone)]
+pub struct RemoteAliasRef {


The name of the struct doesn't make sense to me. This service uses aliases and an optional --to target hint to figure out a unambiguous remote target for a push - so it's not an alias or reference any more.

Should we call it RemoteTarget?

sergiimk · 2024-10-11T16:47:13Z

src/domain/core/src/services/remote_aliases_registry.rs

+    async fn resolve_remote_alias(
+        &self,
+        local_dataset_handle: &DatasetHandle,
+        dataset_push_target_maybe: Option<DatasetPushTarget>,
+        remote_alias_kind: RemoteAliasKind,
+    ) -> Result<RemoteAliasRef, ResolveAliasError>;


I thought that your intent here is to have one generic function that resolves both push and pull targets (thus the RemoteAliasKind parameter to tell which one is it), but DatasetPushTarget kinda ruins that.

Should we call this function resolve_push_target and get rid of remote_alias_kind parameter?

Later we may add resolve_pull_target, but implementation of this service will be free to generalize the logic under the hood if it wants - I think it's more important to keep the interface clear.

sergiimk · 2024-10-11T16:55:16Z

src/app/cli/src/commands/login_command.rs

+        if self.skip_add_repo {
+            return Ok(());
+        }


Such checks are better done at the place of the function call. Seeing add_repository call below I was immediately surprised that the call is unconditional and thought it's a bug.

sergiimk · 2024-10-11T17:03:59Z

src/adapter/http/src/general/dataset_info_handler.rs

+#[serde(rename_all = "camelCase")]
+pub struct DatasetInfoResponse {
+    pub id: DatasetID,
+    pub account_name: Option<AccountName>,


Let's add account_id too. Name is good for visualization, but navigating from dataset to an account should be done by IDs that are resistant to renaming.

sergiimk · 2024-10-11T17:19:10Z

src/infra/core/src/remote_alias_resolver_impl.rs

+                return Err(ResolveAliasError::EmptyRepositoryList);
+            }
+        }
+        let remote_repo = self.remote_repo_reg.get_repository(&repo_name).int_err()?;


If dataset_push_target_maybe == Some(Repository("blah")) and "blah" repository does not exist will we get an internal error here instead of RepositoryNotFound?

sergiimk · 2024-10-11T17:21:21Z

src/infra/core/src/remote_alias_resolver_impl.rs

+        let repo_name: odf::RepoName;
+        let mut account_name = None;
+        let mut dataset_name = None;


Might be better to rewrite this as:

let (repo_name, account_name, dataset_name) = if ... { .. } else { ... };

This way you get certainty that all branches of logic consider all three of these values and not simply forget to assign one of them.

sergiimk · 2024-10-11T17:24:01Z

src/infra/core/src/remote_alias_resolver_impl.rs

+        let transfer_dataset_name = dataset_name.clone().unwrap_or(
+            self.resolve_remote_dataset_name(
+                local_dataset_handle,
+                &remote_repo.url,
+                access_token_maybe.as_ref(),
+            )
+            .await?,
+        );


Isn't resolve_remote_dataset_name an expensive function? Written like this we will always be calling it even if dataset_name was not None.

sergiimk · 2024-10-11T17:33:05Z

src/infra/core/src/remote_alias_resolver_impl.rs

+            .send()
+            .await
+            .int_err()?
+            .error_for_status()


Have you tested this with plain storage repos like S3 or local FS? I don't see any logic that would differentiate those from ODF repos.

Even if a repo has HTTP(S) scheme it's not a fact that it's an ODF node. So we likely need some fallback logic for cases when /info is not present.

sergiimk · 2024-10-11T17:36:57Z

src/infra/core/src/remote_alias_resolver_impl.rs

+    // Return account name if remote workspace is in multi tenant mode
+    pub async fn resolve_remote_account_name(
+        server_backend_url: &Url,
+        access_token_maybe: Option<&String>,


Since you have extracted this helper as a type - you could pass token as constructor argument, not to have to pass it through every function's arguments.

sergiimk · 2024-10-11T17:43:15Z

CHANGELOG.md

+## [Unreleased]
+### Changed
+- `kamu push <dataset>` command now can be called without `--to` reference and Alias or Remote dataset repository will be used as destination
+- `kamu login` command now will store repository to Repository registry. Name can be provided with `--repo-name` flag and to skip creating repo can be used `--skip-add-repo` flag


We also need to check if our instructions on pulling/pushing datasets in the Jupyter demo notebooks need to be updated or can be simplified.

rmn-boiko added 5 commits September 6, 2024 12:34

Improve push command usability

e23dee0

Add account info endpoint

729312f

Merge branch 'master' into feature/450-improve-login-command-ux

ab8451a

Add repo as a to param

9c25603

Fix get account info call

f0481ec

rmn-boiko self-assigned this Sep 26, 2024

rmn-boiko force-pushed the feature/450-improve-login-command-ux branch 2 times, most recently from 131dfc4 to b02576e Compare September 26, 2024 13:34

Merge branch 'master' into feature/450-improve-login-command-ux

7dabc73

rmn-boiko force-pushed the feature/450-improve-login-command-ux branch from b02576e to 7dabc73 Compare September 26, 2024 13:34

rmn-boiko added 7 commits October 1, 2024 13:17

Add new Alisa resolver

b189c38

Finish resolver implementation

51eec38

Merge branch 'master' into feature/450-improve-login-command-ux

93ea8f9

Add workspace info endpoint

48dbc77

Add rest tests

a72186b

Add dataset ref tests

becebcb

Add early return for alias ref

b992cb1

rmn-boiko changed the title ~~Feature/450 improve login command ux~~ Improve login command ux Oct 2, 2024

Fix tests

4b7a016

rmn-boiko requested review from sergiimk, zaychenko-sergei and s373r October 2, 2024 13:08

rmn-boiko added 4 commits October 2, 2024 15:11

Update packages

db0a379

Fix grammar

b32b9cd

Add push protocol test

30872f8

Improve tests

d439086

rmn-boiko force-pushed the feature/450-improve-login-command-ux branch from 39198ea to d439086 Compare October 3, 2024 13:08

Merge branch 'master' into feature/450-improve-login-command-ux

fea8286

rmn-boiko force-pushed the feature/450-improve-login-command-ux branch from 0f23dae to 0f8ffce Compare October 3, 2024 13:47

Fix tests

d6af96e

rmn-boiko force-pushed the feature/450-improve-login-command-ux branch from 0f8ffce to d6af96e Compare October 3, 2024 13:48

Clean code

072f9bf

rmn-boiko marked this pull request as ready for review October 3, 2024 14:07

Merge branch 'master' into feature/450-improve-login-command-ux

7b255e4

s373r reviewed Oct 3, 2024

View reviewed changes

rmn-boiko added 3 commits October 3, 2024 17:18

Fix review comments

b0ec2fc

Update changelog

9b2138d

Add dataset by id rest api tests

546ec2f

sergiimk requested changes Oct 6, 2024

View reviewed changes

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/adapter/http/src/data/account_handler.rs Outdated Show resolved Hide resolved

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/adapter/http/src/data/account_handler.rs Outdated Show resolved Hide resolved

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/adapter/http/src/data/dataset_info_handler.rs Outdated Show resolved Hide resolved

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/adapter/http/src/data/workspace_info_handler.rs Outdated Show resolved Hide resolved

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/infra/core/src/remote_alias_resolver_impl.rs Show resolved Hide resolved

zaychenko-sergei reviewed Oct 7, 2024

View reviewed changes

src/infra/core/src/remote_alias_resolver_impl.rs Outdated Show resolved Hide resolved

rmn-boiko added 2 commits October 8, 2024 15:57

Fix review comments. Iter 1

d87782c

Merge branch 'master' into feature/450-improve-login-command-ux

5d87a52

rmn-boiko requested review from zaychenko-sergei, sergiimk and s373r October 8, 2024 14:05

Fix tests

339c66c

zaychenko-sergei approved these changes Oct 9, 2024

View reviewed changes

sergiimk requested changes Oct 11, 2024

View reviewed changes

sergiimk reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve login command ux #859

Improve login command ux #859

rmn-boiko commented Sep 26, 2024 •

edited

Loading

s373r left a comment

sergiimk left a comment

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

sergiimk Oct 11, 2024

Improve login command ux #859

Are you sure you want to change the base?

Improve login command ux #859

Conversation

rmn-boiko commented Sep 26, 2024 • edited Loading

Description

Checklist before requesting a review

s373r left a comment

Choose a reason for hiding this comment

sergiimk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmn-boiko commented Sep 26, 2024 •

edited

Loading