-
Notifications
You must be signed in to change notification settings - Fork 101
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
UDT - Remove Legacy Routing #8205
base: main
Are you sure you want to change the base?
UDT - Remove Legacy Routing #8205
Conversation
3c22547
to
94382b0
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8205 +/- ##
==========================================
- Coverage 59.83% 59.81% -0.02%
==========================================
Files 590 590
Lines 39520 39513 -7
==========================================
- Hits 23646 23636 -10
+ Misses 14119 14116 -3
- Partials 1755 1761 +6 ☔ View full report in Codecov by Sentry. |
94382b0
to
138974f
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
c8c1be3
to
07f1803
Compare
07f1803
to
cf8042e
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
cf8042e
to
e7ff53c
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
e7ff53c
to
11b4f4f
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
11b4f4f
to
923152d
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
923152d
to
f860504
Compare
@@ -46,15 +44,3 @@ type RadiusPlane struct { | |||
func (p RadiusPlane) ResourceTypeName() string { | |||
return p.Type | |||
} | |||
|
|||
// LookupResourceProvider checks if the input provider is in the list of configured providers. | |||
func (plane RadiusPlane) LookupResourceProvider(key string) string { |
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 for legacy routing to work?
3a0e87d
to
c408ab9
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
c408ab9
to
98ff3c4
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: lakshmimsft <[email protected]>
Signed-off-by: lakshmimsft <[email protected]>
a2dff60
to
cbe8229
Compare
cbe8229
to
65243f6
Compare
65243f6
to
38e1740
Compare
38e1740
to
3e637de
Compare
Signed-off-by: lakshmimsft <[email protected]>
3e637de
to
e5c5e96
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
echo "*** Verify manifests are registered ***" | ||
rm -f registermanifest_logs.txt | ||
# Find the pod with container "ucp" | ||
POD_NAME=$( | ||
kubectl get pods -n radius-system \ | ||
-o jsonpath='{range .items[*]}{.metadata.name}{" "}{.spec.containers[*].name}{"\n"}{end}' \ | ||
| grep "ucp" \ | ||
| head -n1 \ | ||
| cut -d" " -f1 | ||
) | ||
echo "Found ucp pod: $POD_NAME" | ||
|
||
if [ -z "$POD_NAME" ]; then | ||
echo "No pod with container 'ucp' found in namespace radius-system." | ||
exit 1 | ||
fi |
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 shouldn't be the case if the rad install
command above succeeds. There should be a UCP pod for sure. Otherwise, we have a problem.
# Poll logs for up to iterations, 30 seconds each (upto 3 minutes total) | ||
for i in {1..6}; do | ||
kubectl logs "$POD_NAME" -n radius-system | tee registermanifest_logs.txt > /dev/null | ||
|
||
# Exit on error | ||
if grep -qi "error" registermanifest_logs.txt; then | ||
echo "Error found in ucp logs." | ||
exit 1 | ||
fi | ||
|
||
# Check for success | ||
if grep -q "Successfully registered manifests" registermanifest_logs.txt; then | ||
echo "Successfully registered manifests - message found." | ||
break | ||
fi | ||
|
||
echo "Logs not ready, waiting 30 seconds..." | ||
sleep 30 | ||
done |
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 we be looking for specific logs? This will error out in all types of errors.
|
||
# Final check to ensure success message was found | ||
if ! grep -q "Successfully registered manifests" registermanifest_logs.txt; then | ||
echo "Manifests not registered after 3 minutes." |
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.
Shouldn't this be checked during rad install
? Can this case happen after rad install
successfully finishes? As a user, when rad install
shows me success output, I wouldn't expect an error afterwards.
for locationName, address = range resourceProvider.Location { | ||
// We support one location per resourceProvider | ||
break | ||
} |
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.
So, the for loop updates the locationName
with the first location name from the map... We should add a not here.
expectedApiVersion = "2025-01-01-preview" | ||
expectedApiVersion = "2023-10-01-preview" |
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.
Other test resources have 2025 API version (I think and am not sure)
Description
Type of change
Part of: #6688
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: