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

Implement provider declaration aliasing #12127

Merged
merged 21 commits into from Oct 23, 2023
Merged

Implement provider declaration aliasing #12127

merged 21 commits into from Oct 23, 2023

Conversation

asilverman
Copy link
Contributor

@asilverman asilverman commented Oct 12, 2023 •
edited

Overview

Implementation of #11598

Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2023 •
edited

Test this change out locally with the following install scripts (Action run 6615863790)

VSCode
  • Mac/Linux 
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 6615863790
  • Windows 
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 6615863790"
Azure CLI
  • Mac/Linux 
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 6615863790
  • Windows 
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 6615863790"

@asilverman asilverman force-pushed the asilverman/dynamic-type-loading/aliases branch 3 times, most recently from bb88dfe to bb3e086 Compare October 12, 2023 17:31
@asilverman asilverman self-assigned this Oct 12, 2023
@asilverman asilverman added this to the v0.23 milestone Oct 12, 2023
@asilverman asilverman linked an issue Oct 12, 2023 that may be closed by this pull request
Proposal: Azure provider registry aliasing #11598
Closed
@asilverman asilverman added the story: dynamic type loading Collects all work items related to decoupling of Bicep types from compiler label Oct 12, 2023
@asilverman asilverman force-pushed the asilverman/dynamic-type-loading/aliases branch 2 times, most recently from 4480e4e to 16de1e3 Compare October 12, 2023 17:53
@asilverman asilverman force-pushed the asilverman/dynamic-type-loading/aliases branch from 16de1e3 to 0e2742d Compare October 12, 2023 18:09
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2023 •
edited

Test Results

     132 files  ±  0       132 suites  ±0   3h 54m 44s ⏱️ + 3m 48s
10 690 tests +21  10 690 ✔️ +21  0 💤 ±0  0 ❌ ±0 
51 647 runs  +84  51 647 ✔️ +84  0 💤 ±0  0 ❌ ±0 

Results for commit fd93bda. ± Comparison against base commit b5291b4.

♻️ This comment has been updated with latest results.

@asilverman asilverman mentioned this pull request Oct 12, 2023
Open
6 tasks
@asilverman asilverman marked this pull request as ready for review October 13, 2023 20:27
@asilverman asilverman enabled auto-merge (squash) October 15, 2023 18:04
anthony-c-martin
anthony-c-martin previously requested changes Oct 16, 2023
View reviewed changes
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

See comments

@asilverman asilverman force-pushed the asilverman/dynamic-type-loading/aliases branch from edf1af0 to 93bdd3b Compare October 16, 2023 22:31
@asilverman asilverman force-pushed the asilverman/dynamic-type-loading/aliases branch from 93bdd3b to fa45b1a Compare October 17, 2023 17:42
anthony-c-martin
anthony-c-martin reviewed Oct 18, 2023
View reviewed changes
src/Bicep.Core/Syntax/ImportSpecification.cs Outdated

return (name, version, true);
var span = new TextSpan(stringSyntax.Span.Position + 1, unexpandedRepositoryAddress.Length);
var unexpandedArtifactAddress = value.Replace('@', ':'); // we use the same format as module references so we can reuse module dispatcher logic.
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting finding. I'm wondering if it's worth having a quick discussion on the pros/cons of having a syntax with @ to represent the version (clearer), vs having a syntax that fits with the existing module path resolution logic (familiar to existing users)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it's a good point but I worry it's not in scope of this PR. Would you like me to do a write up and bring this up in the bicep discussions forum or do you prefer to do it given it's an insight you have identified?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, it's a good point but I worry it's not in scope of this PR.

Yeah I think it's fine to create a task to track.

anthony-c-martin
anthony-c-martin approved these changes Oct 23, 2023
View reviewed changes
Copy link
Member

@anthony-c-martin anthony-c-martin left a comment

Choose a reason for hiding this comment

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

Looks good!

@asilverman asilverman merged commit 7bccda3 into main Oct 23, 2023
47 checks passed
@asilverman asilverman deleted the asilverman/dynamic-type-loading/aliases branch October 23, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anthony-c-martin anthony-c-martin anthony-c-martin approved these changes

@jeskew jeskew Awaiting requested review from jeskew

@shenglol shenglol Awaiting requested review from shenglol

@majastrz majastrz Awaiting requested review from majastrz

Assignees

@asilverman asilverman

Labels
story: dynamic type loading Collects all work items related to decoupling of Bicep types from compiler
Projects
Bicep
Archived in project
Milestone
v0.23
Development

Successfully merging this pull request may close these issues.

Proposal: Azure provider registry aliasing
5 participants
@asilverman @jeskew @shenglol @majastrz @anthony-c-martin