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.

Sign up for GitHub

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

Jump to bottom

feat: 38589 GitHub App Authentication #15807

Merged
merged 8 commits into from
Feb 19, 2025
Merged

feat: 38589 GitHub App Authentication #15807

merged 8 commits into from
Feb 19, 2025

Conversation

arrestle
Copy link
Contributor

@arrestle arrestle commented Feb 3, 2025

SUMMARY

AAP-385589 Implement GitHub App credential type - Changes needed for UI Testing

ISSUE TYPE
  • New or Enhanced Feature
COMPONENT NAME
  • API
  • Controller
AWX VERSION
ADDITIONAL INFORMATION

@github-actions github-actions bot added component:api dependencies Pull requests that update a dependency file community labels Feb 3, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 3, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.88%. Comparing base (4487f2a) to head (b4ae05f).
Report is 4 commits behind head on devel.

✅ All tests successful. No failed tests found.

@arrestle arrestle requested review from thedoubl3j and AlanCoding and removed request for thedoubl3j February 5, 2025 13:54
thedoubl3j
thedoubl3j reviewed Feb 5, 2025
View reviewed changes
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Feb 13, 2025
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Feb 13, 2025
@arrestle arrestle force-pushed the aap-38589-build-yolo-for-ui branch from 22268c5 to 3eaa0a2 Compare February 14, 2025 14:03
@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Feb 14, 2025
@arrestle arrestle changed the title feat: aap-38589 create yolo build feat: aap-38589 GitHub App Authentication Feb 14, 2025
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Feb 14, 2025
@thedoubl3j thedoubl3j force-pushed the aap-38589-build-yolo-for-ui branch from 007ed10 to a3e33c5 Compare February 17, 2025 18:56
@thedoubl3j thedoubl3j changed the title feat: aap-38589 GitHub App Authentication feat: 38589 GitHub App Authentication Feb 17, 2025
Update git requirements to pull from feature branch
df6da38 
* point git requirements to feature branch for testing purposes
* revert this once the awx-plugins PR merges
@thedoubl3j thedoubl3j force-pushed the aap-38589-build-yolo-for-ui branch from 75be01c to df6da38 Compare February 17, 2025 19:16
fosterseth
fosterseth approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member

@fosterseth fosterseth 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

@arrestle arrestle enabled auto-merge (squash) February 19, 2025 22:06
fosterseth
fosterseth reviewed Feb 19, 2025
View reviewed changes
awx/main/utils/common.py
allowed_git_usernames = {'git', 'x-access-token'}

if scm_type == 'git' and parts.scheme.endswith('ssh'):
is_github_host = parts.hostname in special_hosts or parts.hostname.endswith('.github.com')
Copy link
Member

Choose a reason for hiding this comment

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

seems like we could have just done

if scm_type == 'git' and parts.scheme.endswith('ssh') and parts.hostname in special_git_hosts and not netloc_username in allowed_git_username:

Copy link
Member

Choose a reason for hiding this comment

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

and followup question, does this x-access-token work for bit bucket? if not, do we need to add validation for that case?

@fosterseth fosterseth self-requested a review February 19, 2025 22:54
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@arrestle arrestle merged commit bf4d454 into devel Feb 19, 2025
19 checks passed
@arrestle arrestle deleted the aap-38589-build-yolo-for-ui branch February 19, 2025 23:13
webknjaz
webknjaz reviewed Feb 20, 2025
View reviewed changes
requirements/requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

@fosterseth @thedoubl3j are you not tracking pinned versions of awx-plugins anymore?

Copy link
Member

Choose a reason for hiding this comment

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

This might be something we messed up during this patch @webknjaz lemme look at what we had it set to before and see if it was over written

Copy link
Member

Choose a reason for hiding this comment

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

@thedoubl3j yeah, I figured. I think this got forgotten in some other PR, though. Because I saw it hardcoded to use Git links prior to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz left review comments

@thedoubl3j thedoubl3j thedoubl3j left review comments

@djyasin djyasin djyasin approved these changes

@AlanCoding AlanCoding Awaiting requested review from AlanCoding

@chrismeyersfsu chrismeyersfsu Awaiting requested review from chrismeyersfsu

@fosterseth fosterseth Awaiting requested review from fosterseth

Assignees
No one assigned
Labels
community component:api dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@arrestle @webknjaz @fosterseth @thedoubl3j @djyasin