-
Notifications
You must be signed in to change notification settings - Fork 26
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
Remove httpRoutes from samples #574
Conversation
838f40d
to
d985c88
Compare
quickstarts/dapr/dapr-azure.bicep
Outdated
resource gateway 'Applications.Core/gateways@2023-10-01-preview' = { | ||
name: 'gateway' | ||
properties: { | ||
application: app.id | ||
routes: [ | ||
{ | ||
path: '/' | ||
destination: frontendRoute.id | ||
destination: 'http://frontend:80' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's conventional to omit the 80
when using HTTP since 80
is considered the default port for HTTP (like 443
for HTTPS). Can you update these to look like http://frontend
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -119,15 +86,15 @@ resource webshoppingapigw 'Applications.Core/containers@2023-10-01-preview' = { | |||
properties: { | |||
application: application | |||
container: { | |||
image: 'radius.azurecr.io/eshop-envoy:0.1.4' | |||
image: 'nitcr.azurecr.io/eshop-envoy:0.1.100' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to push a new image to radius cr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, was waiting for the changes to look good, so that I can push the new image to radisu cr.
134d97b
to
1f0fc68
Compare
1f0fc68
to
27efc92
Compare
@@ -67,12 +54,11 @@ resource catalogApi 'Applications.Core/containers@2023-10-01-preview' = { | |||
ASPNETCORE_ENVIRONMENT: 'Development' | |||
ASPNETCORE_URLS: 'http://0.0.0.0:80' | |||
RetryMigrations: 'true' | |||
SeqServerUrl: seqRoute.properties.url | |||
SeqServerUrl: 'http://seq:5340' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to remove this hardcoding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies to all hardcoded strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoding comes as part of using DNS-SD and conventionally making the name of service = name of container. @rynowak had suggested to look at it as the DNS hostname of service, instead of a typical hardcoded value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is desirable hardcoding 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unable to run eshop sample on AKS
was working when I had made the changes long back :( will take a look. |
27efc92
to
88c5256
Compare
It is because the envoy image in ghcr needed to be updated with the contents in this PR.
Will test it on an aks in order to get a public IP next. |
OK, got it to work, please review when you get a chance. @AaronCrawfis @willdavsmith |
@@ -30,7 +30,7 @@ test("eShop on Containers App Basic UI and Functionality Checks", async ({ page | |||
await page.getByText("LOGIN").click(); | |||
|
|||
// Expect page to have proper title | |||
expect(page).toHaveTitle("eShopOnContainers - Identity"); | |||
// expect(page).toHaveTitle("eShopOnContainers - Identity"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a test, now reverted back.
@@ -10,5 +10,5 @@ To build the docker image, run `docker build . -t ghcr.io/radius-project/samples | |||
|
|||
Where NUMBER is one greater than the latest version made. To view versions, see https://ms.portal.azure.com/#view/Microsoft_Azure_ContainerRegistries/RepositoryBlade/id/%2Fsubscriptions%2F66d1209e-1382-45d3-99bb-650e6bf63fc0%2FresourceGroups%2Fassets%2Fproviders%2FMicrosoft.ContainerRegistry%2Fregistries%2Fradius/repository/eshop-envoy. | |||
|
|||
To push the image, run `az acr login -n radius` and then `docker push ghcr.io/radius-project/samples/eshop-envoy:0.1.<NUMBER>`. | |||
To push the image, run `az acr login -n radius` and then `docker push ghcr.io/radius-project/samples/eshop/eshop-envoy:0.1.<NUMBER>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this wrong before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the readme was wrong since all assets related to eshop are in the corrected path.
.DS_Store
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be deleted
3e3c1f4
to
6b103e0
Compare
This pull request has been automatically marked as stale because it has been inactive for 90 days. Remove stale label or comment or this PR will be closed in 7 days. |
@nithyatsu @AaronCrawfis @willtsai - what's still needed here? I can dig in and help if we're stuck. It's hurting us in other places to maintain the |
@rynowak it just needs some priority so that both PM team and I can work on it/ test it to get it merged. |
If the tests are passing can we merge it? @AaronCrawfis do you have feedback that we need to address? |
rewrite eshop and eshop dapr using the http-route-less model radius can support now. Summary of changes
ref: radius-project/radius#6375
The new envoy image is pushed to ghcr. This image had to be edited since the "routes" of gateway is changed to reflect the new names of services. Service names now match the name of the container offering the service. (ref: /radius/samples/reference-apps/eshop/envoy/README.md)
This PR partially fixes: radius-project/radius#6099