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.

Sign up for GitHub

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

Jump to bottom

Assorted doc & mz cli changes for aws/us-west-2 launch #22287

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Assorted doc & mz cli changes for aws/us-west-2 launch #22287

merged 1 commit into from
Oct 12, 2023

Conversation

rjobanp
Copy link
Contributor

@rjobanp rjobanp commented Oct 10, 2023 •
edited
Loading

Motivation

Update docs and the MZ CLI interfaces for the aws/us-west-2 launch. We likely don't want to merge this until the region setup is finalized and ready to roll out.

Tips for reviewer

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 rjobanp requested a review from a team as a code owner October 10, 2023 16:21
@rjobanp rjobanp requested a review from a team October 10, 2023 16:21
@rjobanp rjobanp requested a review from a team as a code owner October 10, 2023 16:21
necaris
necaris reviewed Oct 10, 2023
View reviewed changes
src/mz/tests/e2e.rs Outdated
@@ -617,6 +617,7 @@ mod tests {
status: &'a str,
}

// TODO: add aws/us-west-2 once this test is fixed/re-enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add us-west-2 in this PR, since we're not merging this till launch anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should change this variable and make this assertion work.

The TODO comment is in an old e2e test that is now deadcode (I have another PR for removing it.)

@rjobanp
Copy link
Contributor Author

rjobanp commented Oct 11, 2023

@joacoc updated with your suggestions!

@necaris I actually think this is safe to merge now, the doc changes are pretty minor and it'd be good to get the mzcompose tests to run against this new region sooner rather than later. WDYT?

@maddyblue maddyblue removed their request for review October 11, 2023 16:47
@necaris
Copy link
Contributor

necaris commented Oct 11, 2023

Agree would like the mzcompose tests to get going sooner! I'm happy with it so long as someone with a more customer-facing hat (@ruf-io ? @morsapaes ? @sjwiesman ?) is OK with the public-facing changes including us-west-2 as an available region (albeit not advertised, as you say).

joacoc
joacoc reviewed Oct 12, 2023
View reviewed changes
test/mz-e2e/mzcompose.py Outdated
@@ -208,7 +208,7 @@ def workflow_default(c: Composition, parser: WorkflowArgumentParser) -> None:
output = c.run("mz", "region", "list", "--format", "json", capture=True)
regions = json.loads(output.stdout)
us_region = regions[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs a slight modification.

Suggested change
us_region = regions[0]
for region in regions:
if region["region"] == REGION:
us_region = region

And the two following lines can be removed .

joacoc
joacoc reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@joacoc joacoc left a comment

Choose a reason for hiding this comment

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

Just realized that one part of the test must change.

@rjobanp rjobanp merged commit c171503 into main Oct 12, 2023
20 checks passed
@rjobanp rjobanp deleted the roshan/us-west-2-assorted branch October 12, 2023 18:40
benesch
benesch reviewed Oct 22, 2023
View reviewed changes
doc/user/content/releases/_index.md
@@ -30,6 +30,7 @@ Region | Day of week | Time
--------------|-------------|-----------------------------
aws/eu-west-1 | Wednesday | 2100-2300 [Europe/Dublin]
aws/us-east-1 | Thursday | 0500-0700 [America/New_York]
aws/us-west-2 | Thursday | 0200-0400 [America/Los_Angeles]
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this should reflect the time of the calendar event, not the local time at which the event occurs. I put up #22566 to adjust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joacoc joacoc joacoc approved these changes

@benesch benesch benesch left review comments

@necaris necaris necaris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rjobanp @necaris @benesch @joacoc