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

feat: clean up unused DB table service_provider #2701

Merged

Conversation

peterhaochen47
Copy link
Member

  • This table was added for the UAA-as-SAML-IDP feature (b93c87a)
  • This feature has been removed: Remove SAML IDP feature #2638. Hence this table is now unused.
  • The "DROP TABLE IF EXISTS" syntax would not error out if the table does not exist, compared to just "DROP TABLE".
  • Also clean up docs and a test util that reference this table.

[#182118433]

- This table was added for the UAA-as-SAML-IDP feature
  (b93c87a)
- This feature has been removed:
  #2638. Hence this table is now
  unused.
- The "DROP TABLE IF EXISTS" syntax would not error out if the table
  does not exist, compared to just "DROP TABLE".
- Also clean up docs and a test util that reference this table.

[#182118433]
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186960080

The labels on this github issue will be updated when the story is started.

@peterhaochen47 peterhaochen47 requested review from strehle and a team February 1, 2024 06:15
Copy link
Contributor

@Tallicia Tallicia left a comment

Choose a reason for hiding this comment

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

Looks good

@Tallicia Tallicia added accepted Accepted the issue documentation and removed unscheduled labels Feb 1, 2024
@peterhaochen47 peterhaochen47 requested review from a team and removed request for strehle February 1, 2024 17:39
@Tallicia
Copy link
Contributor

Tallicia commented Feb 1, 2024

@strehle FYI - I believe this is expected to be included before the v77 release as part of the breaking changes for the SAML IdP removal.

@strehle
Copy link
Member

strehle commented Feb 1, 2024

@peterhaochen47 I have seen this also during review of #2638 but I thought you wanted wait with the removal.

My assumption was that maybe we wait an additional release until removing the DB table because if we remove data we cannot revert to the older version.
Assume: someone does a CF update but then realizes that SAML IDP is not available anymore. If they then go back to the UAA before they can run without disruption. If we remove the DB table we remove this possiblity.

Therefore I thought it was wanted to not removing the table, however from my side we can also remove it and then release or
release now 77.v

Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

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

OK for SAP, but see my comments about this step.... if we delete the table, then the way back is not possible

@hsinn0
Copy link
Contributor

hsinn0 commented Feb 1, 2024

I am also for not removing the table right away but waiting for a couple of releases or so.

@peterhaochen47
Copy link
Member Author

Given that this PR has 2 approvals (and that I just discussed this with my team and the team was mostly neutral but leaning toward merging), so I will opt to merge this & include it in UAA v77.0.0.

@peterhaochen47 peterhaochen47 merged commit 8a4ca06 into develop Feb 6, 2024
20 checks passed
@peterhaochen47 peterhaochen47 deleted the pr/develop/clean-up-service-provider-db-table branch February 6, 2024 17:30
@cf-gitbot cf-gitbot removed the accepted Accepted the issue label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants