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

[AAP-12376] Check to make sure only one AWX token exists #280

Closed

Conversation

ddonahue007
Copy link
Contributor

@ddonahue007 ddonahue007 commented May 31, 2023

AAP-12376

Only grab the first token. So checking to make sure we have at least one token makes sense.

@ddonahue007 ddonahue007 requested a review from a team as a code owner May 31, 2023 19:03
Dostonbek1
Dostonbek1 previously approved these changes May 31, 2023
@rooftopcellist
Copy link
Member

One caveat here, is the the user is able to shoot themselves in the foot by making multiple tokens, then not having a clear indication of which token EDA will use. But if this gets tests passing, and we revisit it later, that works too.

@Alex-Izquierdo
Copy link
Contributor

I think we should raise the error but at the creation time. UI team is able to update the UI as well. @jamestalton

If we remove this error but still allowing multiple tokens, the user can not know which token is used. IMO we are not fixing the problem, just moving it to another place.

@mkanoor
Copy link
Contributor

mkanoor commented May 31, 2023

@ddonahue007 I think we should be fixing this when a token is being created here https://github.com/ansible/aap-eda/blob/8ce5460af6ac42d6e9dfb0117f84c467c0246e08/src/aap_eda/api/views/user.py#L135 We should not allow the user to create more than 1 token. The earlier we catch the error the better user experience its going to be. If we allow the user to create multiple tokens and then tell them that you can't use more than 1 token is not right.

@ddonahue007 ddonahue007 requested a review from mkanoor June 2, 2023 21:36
@ddonahue007 ddonahue007 force-pushed the check-one-awx-token-only branch 3 times, most recently from 3bc4ca3 to 2c23afb Compare June 14, 2023 14:28
@ddonahue007 ddonahue007 force-pushed the check-one-awx-token-only branch 2 times, most recently from 25627f6 to d0045ac Compare June 27, 2023 13:10
@ddonahue007 ddonahue007 changed the title Remove check for more than one AWX token. As long as we have one tok… [AAP-12376] Check to make sure only one AWX token exists Jun 28, 2023
@benthomasson
Copy link
Collaborator

Ideally this would be checked with a database constraint. Checking in python is not guaranteed to prevent more than one token in all cases. Although running a database migration in a z-stream release may not be allowed yet. I need to check.

Copy link
Collaborator

@benthomasson benthomasson left a comment

Choose a reason for hiding this comment

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

Add a database constraint or use a transaction to guarantee that only one AWX token exists instead of checking in python.

@benthomasson
Copy link
Collaborator

benthomasson commented Jul 25, 2023

We can also wait and implement support for multiple tokens and avoid this problem together.

@Alex-Izquierdo
Copy link
Contributor

I swear I didn't closed this PR, must be an accident. It can not be reopened, I guess because the fork is private (it returns 404). I'm trying to speak with @ddonahue007 to unblock it.

@ddonahue007
Copy link
Contributor Author

ddonahue007 commented Aug 1, 2023

I opened the fork to be public. I am not able to re-open this PR, maybe I do not have the rights in the project any longer?

@Alex-Izquierdo
Copy link
Contributor

Reopened. Thanks @ddonahue007

@Alex-Izquierdo
Copy link
Contributor

It's very weird, every time I open it its closed automatically.

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.

None yet

6 participants