Skip to content

Fix azure accountinfo#2108

Merged
lionello merged 3 commits into
mainfrom
lio/azure-accountinfo
May 11, 2026
Merged

Fix azure accountinfo#2108
lionello merged 3 commits into
mainfrom
lio/azure-accountinfo

Conversation

@lionello
Copy link
Copy Markdown
Member

@lionello lionello commented May 11, 2026

Description

Azure AccountInfo did not have region set, resulting in the "already deployed elsewhere" prompt.

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • Bug Fixes

    • Fixed BYOC Azure account setup to correctly resolve location and subscription values.
  • Improvements

    • Deployment notifications now show clearer, line-by-line listings of existing deployments.
  • Tests

    • Long-running tests now skip under short test mode to speed up test runs.
  • Chores

    • Simplified pre-push check prerequisites to reduce unnecessary triggers.

Review Change Stack

@lionello lionello requested a review from edwardrf May 11, 2026 19:56
@lionello lionello requested a review from jordanstephens as a code owner May 11, 2026 19:56
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 323b33a4-32d9-4407-8b98-97ad33e25e05

📥 Commits

Reviewing files that changed from the base of the PR and between c9be785 and f617ab7.

📒 Files selected for processing (1)
  • src/pkg/cli/client/byoc/azure/byoc.go

📝 Walkthrough

Walkthrough

This PR trims the pre-push Make prerequisites, adds testing.Short() guards to two long-running tests, changes compose deployment listing output to warning-level and per-line printing, and makes Azure AccountInfo call setUpLocation() before returning account data.

Changes

Pre-push Optimization & Test Short Mode

Layer / File(s) Summary
Build Target Config
Makefile
pre-push target dependency list is reduced by removing test-nix from prerequisites.
Test Short Mode Guards
src/pkg/cli/tailAndMonitor_test.go, src/pkg/cli/waitForCdTaskExit_test.go
TestTailAndMonitor and TestWaitForCdTaskExit now skip when testing.Short() is enabled, avoiding long-running polling and sleep operations in short test runs.

CLI Output & Azure Location Setup

Layer / File(s) Summary
Deployment Listing Output
src/cmd/cli/command/compose.go
Pre-listing message changes from info to warning severity; output is printed line-by-line per deployment instead of as a single newline-joined string.
Azure Location Setup
src/pkg/cli/client/byoc/azure/byoc.go
AccountInfo now calls setUpLocation() before constructing the response, ensuring driver.Location and related fields are resolved from environment variables.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • jordanstephens

Poem

🐰 A faster push, tests aligned with haste,
Deployments now warn in cleaner taste,
Azure locations resolve on demand—
Small tweaks that help the workflow stand!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Fix azure accountinfo' is vague and does not clearly communicate the specific change. While it references Azure AccountInfo, it doesn't explain what the fix entails (region setup) or why it matters. Provide a more specific title like 'Fix Azure AccountInfo region setup' or 'Ensure Azure AccountInfo has Region set' to better clarify the primary change.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lio/azure-accountinfo

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2."
level=warning msg="Suggested new configuration:\nlinters:\n enable:\n - gomodguard_v2\n"
level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
Makefile (1)

13-13: Pre-push simplification makes sense; keep Nix coverage in CI.

Since test-nix was removed from local push hooks, it’s worth ensuring CI still runs it so Nix-only breakages are caught before release.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` at line 13, The local pre-push Makefile target was simplified and
no longer runs the test-nix target, so update CI to still execute Nix-only tests
by adding an explicit step that runs make test-nix (or invokes the test-nix
target directly) in the CI pipeline; look for the CI workflow job that runs
tests and add a step to call the test-nix target (or ensure the existing test
job depends on or invokes test-nix) so Nix-only breakages are caught before
release, referencing the Makefile target name test-nix and the pre-push target
for context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/pkg/cli/client/byoc/azure/byoc.go`:
- Around line 171-173: Wrap the raw error returned by b.setUpLocation() with
contextual AccountInfo using fmt.Errorf and %w so callers can identify the
failing operation; replace the current return nil, err with a wrapped error like
fmt.Errorf("failed to set up location for account %s: %w", b.AccountInfo, err)
(use the actual AccountInfo field or method on b to include identifying info).

---

Nitpick comments:
In `@Makefile`:
- Line 13: The local pre-push Makefile target was simplified and no longer runs
the test-nix target, so update CI to still execute Nix-only tests by adding an
explicit step that runs make test-nix (or invokes the test-nix target directly)
in the CI pipeline; look for the CI workflow job that runs tests and add a step
to call the test-nix target (or ensure the existing test job depends on or
invokes test-nix) so Nix-only breakages are caught before release, referencing
the Makefile target name test-nix and the pre-push target for context.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5e45a09a-4055-4be1-89df-bf90efdc1885

📥 Commits

Reviewing files that changed from the base of the PR and between ed35ec6 and c9be785.

📒 Files selected for processing (5)
  • Makefile
  • src/cmd/cli/command/compose.go
  • src/pkg/cli/client/byoc/azure/byoc.go
  • src/pkg/cli/tailAndMonitor_test.go
  • src/pkg/cli/waitForCdTaskExit_test.go

Comment thread src/pkg/cli/client/byoc/azure/byoc.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@lionello lionello merged commit 0afb8f6 into main May 11, 2026
3 of 5 checks passed
@lionello lionello deleted the lio/azure-accountinfo branch May 11, 2026 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants