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.

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

fix(decorator): replace hard code url with {endpoint} in @scenarioService #598

Merged

Conversation

archerzz
Copy link
Member

Hard coded value should be replaced by a parameter, like #425

fix #597

Cadl Ranch Contribution Checklist:

  • I have written a scenario spec
  • I have meaningful @scenario names. Someone can look at the list of scenarios and understand what I'm covering.
  • I have written a mock API
  • I have used @scenarioDocs for extra scenario description and to tell people how to pass my mock api check.

…Service`

Hard coded value should be replaced by a parameter, like Azure#425

fix Azure#597
Copy link

changeset-bot bot commented Jun 13, 2024

🦋 Changeset detected

Latest commit: 8e9ca17

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@azure-tools/cadl-ranch-expect Patch
@azure-tools/cadl-ranch-specs Patch
@azure-tools/cadl-ranch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jun 13, 2024

Any other language need this?

For Java, this change means we need to update all the client in test (probably on the hundreds) to set endpoint=localhost:3000

@archerzz
Copy link
Member Author

Any other language need this?

For Java, this change means we need to update all the client in test (probably on the hundreds) to set endpoint=localhost:3000

I believe dotnet is not using localhost:3000: https://github.com/Azure/autorest.csharp/blob/9790814dd86eb1279e5c98fd123e4bcea7456553/test/CadlRanchProjects.Tests/type-enum-fixed.cs#L14-L18

But dotnet allows users to override the host, even if it's explicitly specified in spec. Things could be different in other languages. We can hold this PR for further discussion.

Copy link
Member

@pshao25 pshao25 left a comment

Choose a reason for hiding this comment

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

LGTM, can we make its default value as "http://localhost:3000" so that it won't break others.

@qiaozha
Copy link
Member

qiaozha commented Jun 13, 2024

LGTM, can we make its default value as "http://localhost:3000" so that it won't break others.

I don't think we can, because currently there's no way to set client side default value in typespec.

@tadelesh
Copy link
Member

tadelesh commented Jun 14, 2024

LGTM, can we make its default value as "http://localhost:3000" so that it won't break others.

I don't think we can, because currently there's no way to set client side default value in typespec.

for endpoint, tcgc will treat service default as client default. this is the only exception. mpg template also has such pattern. @archerzz could you help to refine it?

@weidongxu-microsoft
Copy link
Member

for endpoint, tcgc will treat service default as client default. this is the only exception. mpg template also has such pattern. @archerzz could you help to refine it?

I see :-)

Yeah, if we all take the service default of "host" as client default, I am good with this PR, as long as it have localhost as default of the endpoint.

@archerzz
Copy link
Member Author

Please hold this PR. After discussion, we decided to inject the following:

@server(
  "{endpoint}",
  "Test server endpoint",
  {
    endpoint: string = "http://localhost:3000",
  }
)

blocked by Azure/typespec-azure#1030

@archerzz archerzz force-pushed the decorator/fix-hard-code-url-scenarioService branch from 04f02e5 to 3142134 Compare July 29, 2024 07:14
@archerzz archerzz force-pushed the decorator/fix-hard-code-url-scenarioService branch from 3142134 to 7e6bff4 Compare July 29, 2024 07:15
@archerzz
Copy link
Member Author

Note, this is blocked by Azure/typespec-azure#1030

@tadelesh
Copy link
Member

@archerzz i believe you could merge this pr. current tcgc will do the same thing.

@iscai-msft iscai-msft enabled auto-merge (squash) August 23, 2024 16:51
@iscai-msft iscai-msft merged commit 039ab07 into Azure:main Aug 23, 2024
9 checks passed
archerzz pushed a commit to archerzz/cadl-ranch that referenced this pull request Sep 2, 2024
archerzz added a commit that referenced this pull request Sep 2, 2024
…scenarioService` (#598)" (#719)

* Revert "fix(decorator): replace hard code url with `{endpoint}` in `@scenarioService` (#598)"

This reverts commit 039ab07.

* add changeset

---------

Co-authored-by: Mingzhe Huang (from Dev Box) <[email protected]>
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.

bug(decorator): hard coded server url in @scenarioService
6 participants