Skip to content

cli: warn on ambiguous 2-part environment refs#617

Open
LouisLau-art wants to merge 2 commits intopulumi:mainfrom
LouisLau-art:fix/warn-ambiguous-env-ref
Open

cli: warn on ambiguous 2-part environment refs#617
LouisLau-art wants to merge 2 commits intopulumi:mainfrom
LouisLau-art:fix/warn-ambiguous-env-ref

Conversation

@LouisLau-art
Copy link

Fixes #613.

When a user belongs to an org named a, a 2-part env reference like a/b can be interpreted as either:

  • <project>/<environment> (default org + project a + env b), or
  • <org>/default/<environment> (org a + default project + env b).

This adds a warning in that case, recommending the unambiguous 3-part form <org>/<project>/<environment>.

Tests:

  • go test ./cmd/esc/cli -run TestCLI

Copy link
Contributor

@seanyeh seanyeh left a comment

Choose a reason for hiding this comment

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

thanks for contributing! I have a few small comments

return s
}

func (cmd *envCommand) warnIfAmbiguousTwoPartRef(ctx context.Context, refString string, ref environmentRef) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not do any org checking in this method - we already do this elsewhere (look at the other comments)
This method can simply print out a warning

return
}

if cmd.warnedAmbiguousRefs == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary

return ref, nil
}

cmd.warnIfAmbiguousTwoPartRef(ctx, refString, ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

we do the org-checking below - you can move this call to inside the if existsLegacyPath { ... } check

return ref, args, nil
}

cmd.warnIfAmbiguousTwoPartRef(ctx, cmd.envNameFlag, ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

create a new check after we calculate existsLegacyPath below, and move this call there. something like:

if existsLegacyPath {
  cmd.warnIfAmbiguousTwoPartRef(...)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding a test!

@LouisLau-art
Copy link
Author

Thanks for the detailed review comments. I’ll push a follow-up that:

  1. removes org checking from the warning helper,
  2. moves the warning call under the existsLegacyPath branch (after it is computed),
  3. drops the unnecessary piece you called out and keeps the test update.

I’ll post again after the patch is up.

@LouisLau-art
Copy link
Author

Pushed follow-up commit 5927edc with the requested changes:

  1. warnIfAmbiguousTwoPartRef now only prints the warning message (no org membership checks).
  2. Warning calls were moved to run only after existsLegacyPath is determined:
    • getNewEnvRef: warn inside if existsLegacyPath { ... }
    • getExistingEnvRefWithRelative: warn inside if existsLegacyPath { ... }
  3. Removed the warning de-dup map from envCommand.

Test updates:

  • testPulumiClient.EnvironmentExists now checks the in-memory environment map.
  • Updated env-ambiguous-two-part-warning.yaml to model the legacy-path resolution case.

Validation:

  • go test ./cmd/esc/cli -run TestCLI -count=1

)

if existsLegacyPath {
cmd.warnIfAmbiguousTwoPartRef(refString)
Copy link
Contributor

Choose a reason for hiding this comment

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

one last comment: we already have the same check below (see line 306) - let's move this warning there

@LouisLau-art
Copy link
Author

Thanks for the final note — agreed. I’ll move that warning to the shared check so we only emit it once and avoid duplicate logic.

@LouisLau-art
Copy link
Author

Clarification: I’ll move the warning call under the shared existsLegacyPath check so the warning is emitted once from a single place.

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.

Give warning when ambiguous 2-part name (a/b) is used

2 participants