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

fix value error if there is an issue with initializing domain #35819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

biyeun
Copy link
Member

@biyeun biyeun commented Feb 20, 2025

Technical Summary

the delete function will always throw a ValueError (unless in UNIT_TESTING mode) due to

if not leave_tombstone and not settings.UNIT_TESTING:
raise ValueError(
'Cannot delete domain without leaving a tombstone except during testing')

as a result the following exception (which the UI knows how to handle properly) is never raised

Safety Assurance

Safety story

safe change, also tested locally

Automated test coverage

no

QA Plan

Not needed

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@biyeun biyeun added the product/invisible Change has no end-user visible impact label Feb 20, 2025
@dimagimon dimagimon added the Risk: High Change affects files that have been flagged as high risk. label Feb 20, 2025
millerdev
millerdev previously approved these changes Feb 21, 2025
@@ -170,7 +170,7 @@ def request_new_domain(request, project_name, is_new_user=True, is_new_sso_user=
'first_domain_for_user': is_new_user,
'error': str(error),
})
new_domain.delete()
new_domain.delete(leave_tombstone=True)
Copy link
Contributor

@millerdev millerdev Feb 21, 2025

Choose a reason for hiding this comment

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

Will this make it impossible to create a domain with the same name in the future? If yes, that seems not great.

Copy link
Member Author

Choose a reason for hiding this comment

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

@millerdev it's currently impossible, as the domain is never actually deleted due to the ValueError being thrown. I'm just trying to fix the error here, but I think taking a different approach (full delete and working around the domain deletion checks) can be done later. I don't have time at the moment to dedicate to this.

@millerdev millerdev dismissed their stale review February 21, 2025 13:46

Thought of a question that suggests maybe there is a better way to do this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact Risk: High Change affects files that have been flagged as high risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants