Skip to content

Conversation

@rohan-dassani
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

rohan-dassani and others added 30 commits August 26, 2022 13:16
	modified:   src/connectedk8s/azext_connectedk8s/_params.py
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	new file:   src/connectedk8s/azext_connectedk8s/patch-file.yaml
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/azext_connectedk8s/commands.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	new file:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	renamed:    src/connectedk8s/azext_connectedk8s/patch-file.yaml -> src/connectedk8s/azext_connectedk8s/remove_crd_finalizer.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/setup.py
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/setup.py
@azure-client-tools-bot-prd
Copy link

Hi @rohan-dassani,
If you want to release the new extension version.
Please write the description of changes into HISTORY.rst and update setup.py.

@azure-client-tools-bot-prd
Copy link

Hi @rohan-dassani,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

@yonzhan
Copy link
Collaborator

yonzhan commented May 16, 2023

Thank you for your contribution! We will review the pull request and get back to you soon.

@rohan-dassani rohan-dassani marked this pull request as draft May 16, 2023 14:19
@yonzhan yonzhan requested review from yanzhudd and zhoxing-ms May 16, 2023 16:01
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/_constants.py
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
@@ -212,6 +212,9 @@
Cluster_Diagnostic_Checks_Job_Log_Save_Failed = 'Failed to save cluster diagnostic checks job log'
# Diagnostic Results Name
Outbound_Connectivity_Check_Result_String = "Outbound Network Connectivity Result:"
Copy link
Contributor

@sirireddy12 sirireddy12 May 19, 2023

Choose a reason for hiding this comment

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

I think this is a little confusing. Let's name the tests as "Outbound_Connectivity_Check_For_Onboarding" and "Outbound_Connectivity_Check_For_ClusterConnect" at all places to make it clear in the code as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment for logs as well which we're appending in the arc_diagnostic_logs folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but OBO is not the only endpoint for cluster connect right , so if any other endpoint required for cluster connect fails then the log will not give the right impression to us and to the user.

# Validating if outbound connectiivty is working or not and displaying proper result
if(outbound_connectivity_response != "000"):
# since connectivity to obo endpoint is required only for cluster connect feature , we only give out a warning if connectivity to obo endpoint is not present
if obo_check_response_code == "000":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have this condition outside of the parent if check? That way we keep it independent and will be easy to increase the list of endpoints in each category easily in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for keeping this inside the if condition was , that supposing that the outbound connectivity isnt there to any endpoint , it will still give out the warning about obo endpoint and then it will say outbound connectivity fail when it checks the mcr endpoint.

So i thought if mcr itself fails , then pointing to obo specifically doesnt make sense and the user should look into all the endpoint network requirements document. And if mcr connectivity is there than pointing out if some specific endpoint is not listed makes more sense.

@rohan-dassani rohan-dassani changed the title OBO changes + CRD cleanup + helm client download retires CRD cleanup + helm client download retires + error classification May 19, 2023
Copy link
Contributor

@sirireddy12 sirireddy12 left a comment

Choose a reason for hiding this comment

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

lgtm

@rohan-dassani rohan-dassani marked this pull request as ready for review May 19, 2023 09:59
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
@zhoxing-ms zhoxing-ms merged commit 129d446 into Azure:main May 22, 2023
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ connectedk8s ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=57185&view=results

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.

6 participants