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

[HOLD] Throw error if reverse-sync needed, but not allowed #619

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

Conversation

AlanCoding
Copy link
Member

This is a re-do of #618 that attempts to answer the question "what if someone runs awx-manage createsuperuser manually?"

If that command is ran with explicit disabling of reverse-sync, then it will be allowed. It will be confusing that doing this is still permitted even through local_resource_management is off.

But what's the most wrong is to have

  • ALLOW_LOCAL_RESOURCE_MANAGEMENT False
  • reverse sync enabled (multiple mechanisms involved)

The argument is that this configuration is so dangerously contradictory that we should throw an error so that we can front-load debugging of the situation.

@relrod
Copy link
Member

relrod commented Sep 26, 2024

Comments:

  • Sonar can be ignored here, we use Exception above this anyway.
  • I like this approach better than [Alt 1] Flip default of reverse sync to True #618, this has my vote.
  • Can we add a quick unit test for this in test_app/tests/resource_registry/test_utils.py

Beyond that, this LGTM.

@AlanCoding AlanCoding added the Ready for review This PR is ready for review either initially or comments have been address label Oct 2, 2024
@AlanCoding AlanCoding added blocked Can not be done right now for some reason, but want to do later and removed Ready for review This PR is ready for review either initially or comments have been address labels Oct 7, 2024
@AlanCoding
Copy link
Member Author

I identified a conflict with awx-operator, and probably more. This would probably break things, and our viable path forward may be to go back to Alt 1 out of necessity.

@AlanCoding AlanCoding changed the title [Alt 2] Default to reverse-sync, throw error if not allowed Throw error if reverse-sync needed, but not allowed Oct 9, 2024
Thow an error if shared fields are edited with local management turned off

Fix grammar error

Co-authored-by: Rick Elrod <[email protected]>

Add test and change to ValidationError

Disconnect signals to fix failure
Copy link

sonarcloud bot commented Oct 11, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
50.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@john-westcott-iv john-westcott-iv changed the title Throw error if reverse-sync needed, but not allowed [HOLD] Throw error if reverse-sync needed, but not allowed Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Can not be done right now for some reason, but want to do later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants