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

[Databricks E2E] [BUG]: Clean Up should Delete App Registration #1019

Merged

Conversation

elenaterenzi
Copy link
Contributor

Type of PR

  • Code changes

Purpose

  • currently clean_up.sh does not delete app registrations, but only enterprise apps. This PR adds code to deal with app registration removal

Does this introduce a breaking change? If yes, details on what can break

Author pre-publish checklist

  • Added test to prove my fix is effective or new feature works
  • No PII in logs
  • Made corresponding changes to the documentation

Validation steps

  • ...

Issues Closed or Referenced

  • References #issue_number

@elenaterenzi elenaterenzi self-assigned this Jan 14, 2025
@elenaterenzi elenaterenzi changed the base branch from main to e2e/databricks/parking-sensors-V1 January 14, 2025 14:29
@elenaterenzi elenaterenzi changed the title [Databricks E2E] [BUG]: [Databricks E2E] [BUG]: Clean Up should Delete App Registration Jan 14, 2025
Copy link
Collaborator

@ydaponte ydaponte left a comment

Choose a reason for hiding this comment

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

Approved!

@elenaterenzi elenaterenzi merged commit 442cf6e into e2e/databricks/parking-sensors-V1 Jan 16, 2025
3 checks passed
@elenaterenzi elenaterenzi deleted the elena/clean-up-remove-ad-apps branch January 17, 2025 18:29
ydaponte added a commit that referenced this pull request Jan 21, 2025
* [E2E Databricks] - Automate region vm + dbx type cluster in the region (#988)

* automate region vm + dbx type cluster in the region

* Update e2e_samples/parking_sensors/scripts/configure_databricks.sh

Co-authored-by: Elena Terenzi <[email protected]>

* filtering photon support

---------

Co-authored-by: Elena Terenzi <[email protected]>

* [Databricks E2E Sample] - bug fix dbx compute (#1006)

* fix breaking change from dbx cli output
* fix hyperlink check

* Cluster config update (#1012)

* Update clean_up.sh

Adding the Cleanup for federal credentials which follows a certain order:
Get the SP Objc ID of the Service Connection
Delete federated credentials from the SP Object ID
Delete the service connection

* Update deploy_azdo_service_connections_azure.sh

Service Connections are using the Workload Identity which comes from a configuration file.

The cleanup in case of re-start of this script also follows a peculiar order: 

Get SP object if according to the Service Connection
Get the federate credential according to the SP
Delete federated credentials
Delete the Service Connection

* Update README.md

changing the docs with the required permission

* Update README.md

rephrase the permissions needed for azdevops project

* Update e2e_samples/parking_sensors/scripts/clean_up.sh

Co-authored-by: Yennifer Santos <[email protected]>

* Update e2e_samples/parking_sensors/scripts/deploy_azdo_service_connections_azure.sh

Co-authored-by: Yennifer Santos <[email protected]>

* Update README.md

I adjust the order of the permissions

* Update README.md

adding know issues how to delete federate credentials

* Update README.md

identation was not correct

* Fix The property "options" Warnings in Dashboard.bicep

* Update deploy_azdo_service_connections_azure.sh

adding the following changes:
1) Function for cleanup
2)Function for sleep
3)Remove log file

* Update clean_up.sh

Implement function for cleanup and function for sleep

* Update common.sh

Added cleanup_federate_credentials and wait_for_credentials used in the clean_up.sh and deploy_azdo_service_connection.sh

* Update clean_up.sh

rename wait_for_cleanup to wait_for_process

* Update deploy_azdo_service_connections_azure.sh

renamed wait_for_cleanup to wait_for_process

* Fix typo in clean_up.sh log message

typo:
comtain to contain fxed

* Update e2e_samples/parking_sensors/README.md

added Elena's suggestion of rewrite

Co-authored-by: Elena Terenzi <[email protected]>

* Update common.sh

remove the log for SP

* Update deploy_azdo_service_connections_azure.sh

Clean up of the log
suppress update pipelines message
remove response from logging

* Update common.sh

changing the log for federate credentials removal

* Update common.sh

adding the SP in the log message

* Update deploy_azdo_service_connections_azure.sh

checking if the file exist before removal.

* Update e2e_samples/parking_sensors/scripts/common.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/common.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/common.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/deploy_azdo_service_connections_azure.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/deploy_azdo_service_connections_azure.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/clean_up.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/common.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Update e2e_samples/parking_sensors/scripts/common.sh

Co-authored-by: Elena Terenzi <[email protected]>

* Remove unnesissary dependsOn in main.bicep

* Remove redundant comment and variable

role unecessary

* Checking if I can solve the conflicts

* conflict

* conflict

* The new file after tryto solve the conflict

* Trying again to solve the conflict

* Suppress output for service connection deletion

adding -0 none to resolve conflict

* add code to delete app registrations (#1019)

* [Databricks E2E] [BUG] cleanup fix bug with federated credential removal (#1047)

* remove federated credentials only when needed

* fix typo

---------

Co-authored-by: Elena Terenzi <[email protected]>
Co-authored-by: Ayman El-Ghazali <[email protected]>
Co-authored-by: LiliamLeme <[email protected]>
Co-authored-by: Raafat Zarka <[email protected]>
Co-authored-by: Liliam Leme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: clean up script should clean up app registrations, and not just enterprise apps
2 participants