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

Removal of OAuth2 stuff from CLI #15613

Open
wants to merge 10 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Nov 4, 2024

SUMMARY

Stuff this does

  • remove CLI login subcommand
  • remove the --conf.token option from the CLI
  • remove applications and tokens as resources in CLI, and from import/export
    • bleeds over into the collection, as applications is duplicated in options for collection module
  • cascade delete some supporting code in the awxkit library

Ping @gravesm

ISSUE TYPE
  • Breaking Change
COMPONENT NAME
  • CLI
COMPONENT NAME
  • API

@github-actions github-actions bot added component:cli component:awx_collection issues related to the collection for controlling AWX labels Nov 4, 2024
@AlanCoding
Copy link
Member Author

Highly related to: #15623

@AlanCoding AlanCoding changed the title [WIP] Removal of OAuth2 stuff from CLI Removal of OAuth2 stuff from CLI Nov 12, 2024
@AlanCoding AlanCoding marked this pull request as ready for review November 12, 2024 13:20
@AlanCoding
Copy link
Member Author

Can't get verification from integration tests due to unrelated issues.

CI failures I have addressed in other open PRs. Marked as ready for review, otherwise, merge blocked on totally unrelated stuff.

@@ -86,11 +86,6 @@
- workflow names, IDs, or named URLs to export
type: list
elements: str
applications:
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not remove the applications module from the Collection PR. I thought it was used for more purposes apart from Oauth. Is it not?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think applications serve any purpose other than for creating tokens. IMO, the tests seemed to reflect this. It's not impossible that I'm wrong, but it seemed relatively clear even from reading the issue for this PR.

Copy link
Collaborator

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

Just double checking to be thorough, should we remove any of the following?

awxkit/awxkit/api/pages/base.py Outdated Show resolved Hide resolved
@AlanCoding
Copy link
Member Author

@hakbailey thanks, these are good suggestions. However, I have been trying to follow the de-referencing cascade from removing those, which goes deep into the fixture structure of integration tests. This gets a little crazy. My head is spinning from the distinction of v2.load_session() and v2.connection.login(), and if the distinction between those should remain after removing tokens.

I deleted the method in awxkit

def uses_sessions(connection):
    session_login = connection.get(f"{config.api_base_path}login/")
    return session_login.status_code == 200

But this is really my critical point of uncertainty. Why would, or would not, the login endpoint give a 200 code? I thought this would have been part of the recent resource-server work, but no, the method appears to be positively ancient. Previously, it should have always given a 200.

I'm thinking perhaps this goes too far, since AWXKIT_SESSIONS is its own independent configuration. You could use a session, use basic auth, or use a token. The session/not-session is still valid? I think...

@AlanCoding
Copy link
Member Author

@hakbailey I pushed a commit that carries out your suggestions. This isn't super-trivial, because there were other method references beyond just the things you mention, and there are many corresponding integration test changes that also cascade from those.

Copy link
Collaborator

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

This looks good to me assuming the integration test updates worked...

Copy link

sonarcloud bot commented Nov 13, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:awx_collection issues related to the collection for controlling AWX component:cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants