Update MZ CLI to use new Region API version 1 semantics #21871

rjobanp · 2023-09-21T14:56:25Z

Motivation

This PR adds a known-desirable feature: A follow-on to https://github.com/MaterializeInc/cloud/issues/7429 to update the mz cli to use the new Region API response semantics introduced with api version 1.

Tips for reviewer

I tested the mz region list, mz region enable, and mz region disable commands against staging. Let me know if there are other tests to include. I wasn't able to test using the e2e test which looks like it's currently disabled.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

rjobanp · 2023-09-21T14:57:55Z

src/mz/README.md

@@ -134,7 +134,7 @@ Field             | Type   | Description
 `app-password`    | string | *Secret.* The app password to use for this profile.
 `region`          | string | The default region to use for this profile.
 `vault`           | string | The vault to use for this profile. See [Global parameters](#global-parameters) above.
-`api-endpoint`    | string | *Internal use only.* The Materialize API endpoint to use.
+`cloud-endpoint`  | string | *Internal use only.* The Materialize API endpoint to use.


this was a typo I found when making a profile that used staging

RobinClowers · 2023-09-21T18:08:16Z

src/mz/src/command/region.rs

+                    Err(Error::NotReadyRegion)
+                }
+                RegionState::DeletionPending | RegionState::SoftDeleted => Err(
+                    Error::CommandExecutionError("Error enabling region".to_string()),


I wonder if we want to be more explicit for these cases? "This region has been scheduled for deletion," or something similar?

+1. We should. It helps to debug faster and provide more context/value to the user.

RobinClowers

Makes sense conceptually, but I didn't try to test it out.

ParkMyCar

LGTM!

ParkMyCar · 2023-09-21T20:53:03Z

src/cloud-api/src/client.rs

@@ -70,13 +71,23 @@ impl Client {
        method: Method,
        path: P,
        cloud_provider: &CloudProvider,
+        api_version: Option<u16>,


Can you document the behavior of what happens if we don't specify an api_version?

joacoc

Looks good to me. Just a few comments about the release process/rust docs.

joacoc · 2023-09-22T10:11:23Z

src/cloud-api/src/client/region.rs

    /// may not be set if the region is in the process
-    /// of being created
+    /// of being created (see RegionState for details)


Suggested change

/// of being created (see RegionState for details)

/// of being created (see [RegionState] for details)

joacoc · 2023-09-22T10:23:07Z

src/mz/Cargo.toml

@@ -2,7 +2,7 @@
 name = "mz"
 description = "The Materialize command-line interface (CLI)."
 license = "Apache-2.0"
-version = "0.2.1"
+version = "0.2.2"


Suggested change

version = "0.2.2"

version = "0.2.1"

joacoc · 2023-09-22T10:30:42Z

src/mz/CHANGELOG.md

@@ -3,6 +3,14 @@
 All notable changes to the `mz` CLI will be documented in this file.


+## [0.2.2] - 2023-09-21


Many thanks for writing this 🙏 - We should remove the comments in the changelog for this PR. To keep the release process as is. (I will re-add them in the release PR. I have to add other changes, and trigger the release process. We can do it as soon as we merge this.)

joacoc · 2023-09-22T10:48:27Z

src/cloud-api/src/client/region.rs

+    Enabled,
+
+    /// Enablement Pending
+    /// regionInfo field will be null while the region is in this state


Suggested change

/// regionInfo field will be null while the region is in this state

/// [region_info][Region::region_info] field will be `null` while the region is in this state

joacoc · 2023-09-22T10:48:34Z

src/cloud-api/src/client/region.rs

+    EnablementPending,
+
+    /// Deletion Pending
+    /// regionInfo field will be null while the region is in this state


Suggested change

/// regionInfo field will be null while the region is in this state

/// [region_info][Region::region_info] field will be null while the region is in this state

joacoc · 2023-09-22T10:48:42Z

src/cloud-api/src/client/region.rs

+    DeletionPending,
+
+    /// Soft deleted; Pending hard deletion
+    /// regionInfo field will be null while the region is in this state


Suggested change

/// regionInfo field will be null while the region is in this state

/// [region_info][Region::region_info] field will be null while the region is in this state

joacoc · 2023-09-22T11:07:21Z

src/mz/src/command/region.rs

+                    Err(Error::NotReadyRegion)
+                }
+                RegionState::DeletionPending | RegionState::SoftDeleted => Err(
+                    Error::CommandExecutionError("Error enabling region".to_string()),


+1. We should. It helps to debug faster and provide more context/value to the user.

rjobanp · 2023-09-22T22:09:38Z

Thank you all for the comments! I believe I've addressed them all now

joacoc · 2023-09-25T13:18:40Z

@rjobanp LGTM!

rjobanp requested review from RobinClowers and joacoc September 21, 2023 14:56

rjobanp requested a review from a team as a code owner September 21, 2023 14:56

rjobanp requested review from a team and ParkMyCar September 21, 2023 14:56

rjobanp assigned joacoc Sep 21, 2023

rjobanp force-pushed the roshan/request-headers branch from 08058b7 to 02f52ac Compare September 21, 2023 14:57

rjobanp commented Sep 21, 2023

View reviewed changes

RobinClowers reviewed Sep 21, 2023

View reviewed changes

RobinClowers approved these changes Sep 21, 2023

View reviewed changes

ParkMyCar approved these changes Sep 21, 2023

View reviewed changes

joacoc approved these changes Sep 22, 2023

View reviewed changes

Update MZ CLI to use new Region API version 1 semantics

5034fae

rjobanp force-pushed the roshan/request-headers branch from 02f52ac to 5034fae Compare September 22, 2023 22:09

rjobanp merged commit 630c7c9 into main Sep 25, 2023

rjobanp deleted the roshan/request-headers branch September 25, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update MZ CLI to use new Region API version 1 semantics #21871

Update MZ CLI to use new Region API version 1 semantics #21871

rjobanp commented Sep 21, 2023 •

edited

Loading

rjobanp Sep 21, 2023

RobinClowers Sep 21, 2023

joacoc Sep 22, 2023

RobinClowers left a comment

ParkMyCar left a comment

ParkMyCar Sep 21, 2023

joacoc left a comment

joacoc Sep 22, 2023

joacoc Sep 22, 2023

joacoc Sep 22, 2023

joacoc Sep 22, 2023

joacoc Sep 22, 2023

joacoc Sep 22, 2023

joacoc Sep 22, 2023

rjobanp commented Sep 22, 2023

joacoc commented Sep 25, 2023

	/// of being created (see RegionState for details)
	/// of being created (see [RegionState] for details)

		@@ -3,6 +3,14 @@
		All notable changes to the `mz` CLI will be documented in this file.


		## [0.2.2] - 2023-09-21

	/// regionInfo field will be null while the region is in this state
	/// [region_info][Region::region_info] field will be `null` while the region is in this state

Update MZ CLI to use new Region API version 1 semantics #21871

Update MZ CLI to use new Region API version 1 semantics #21871

Conversation

rjobanp commented Sep 21, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobinClowers left a comment

Choose a reason for hiding this comment

ParkMyCar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joacoc 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

rjobanp commented Sep 22, 2023

joacoc commented Sep 25, 2023

rjobanp commented Sep 21, 2023 •

edited

Loading