-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add ability to update scm project using github app #15472
base: devel
Are you sure you want to change the base?
Conversation
- Add github_app_id, github_app_installation_id and github_api_url to scm credential type - Add ability to generate github app token to clone project with git for github
6187010
to
f8e9862
Compare
Quality Gate passedIssues Measures |
This should be a different credential. |
@@ -645,6 +645,9 @@ def create(self): | |||
{'id': 'password', 'label': gettext_noop('Password'), 'type': 'string', 'secret': True}, | |||
{'id': 'ssh_key_data', 'label': gettext_noop('SCM Private Key'), 'type': 'string', 'format': 'ssh_private_key', 'secret': True, 'multiline': True}, | |||
{'id': 'ssh_key_unlock', 'label': gettext_noop('Private Key Passphrase'), 'type': 'string', 'secret': True}, | |||
{'id': 'github_app_id', 'label': gettext_noop('GitHub App ID'), 'type': 'string'}, | |||
{'id': 'github_app_installation_id', 'label': gettext_noop('GitHub App Installation ID'), 'type': 'string'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to surface this into a UI field? This is usually discoverable automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
automatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ideally, there should be no UI inputs at all. Log into https://pre-commit.ci or Codecov / RTD to see their UX for working with repos accessible through GH app installations.
Whenever the app is installed, you already get an event with all the information needed via webhooks.
@@ -645,6 +645,9 @@ def create(self): | |||
{'id': 'password', 'label': gettext_noop('Password'), 'type': 'string', 'secret': True}, | |||
{'id': 'ssh_key_data', 'label': gettext_noop('SCM Private Key'), 'type': 'string', 'format': 'ssh_private_key', 'secret': True, 'multiline': True}, | |||
{'id': 'ssh_key_unlock', 'label': gettext_noop('Private Key Passphrase'), 'type': 'string', 'secret': True}, | |||
{'id': 'github_app_id', 'label': gettext_noop('GitHub App ID'), 'type': 'string'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also surfaced to the end-users? Why? It should be a platform-global value, it's not directly a credential.
{ | ||
'iat': int(time.time()), # Issued at time | ||
'exp': int(time.time()) + (10 * 60), # JWT expiration time (10 minute maximum) | ||
'iss': project_update.credential.get_input('github_app_id', default=''), # GitHub App's identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GH App should be global for the entire AWX. The whole idea is that the users don't need to set up and maintain multiple apps. They just need to click “Install” on GH UI.
|
||
headers = {'Authorization': f'Bearer {jwt_token}', 'Accept': 'application/vnd.github.v3+json'} | ||
|
||
github_api_url = project_update.credential.get_input('github_api_url', default='https://api.github.com') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as well should be a deployment-global value rather than bound to a credential.
On @chrismeyersfsu 's point, the intent was never for the awx/awx/main/models/projects.py Lines 163 to 164 in 79c1921
Instead, it only wants the awx/awx/main/models/credential/__init__.py Lines 135 to 137 in 79c1921
Again, I think that's wrong. When the awx/awx/main/models/credential/__init__.py Line 362 in 79c1921
you can see the choices: awx/awx/main/models/credential/__init__.py Lines 347 to 360 in 79c1921
This |
headers = {'Authorization': f'Bearer {jwt_token}', 'Accept': 'application/vnd.github.v3+json'} | ||
|
||
github_api_url = project_update.credential.get_input('github_api_url', default='https://api.github.com') | ||
installation_id = project_update.credential.get_input('github_app_installation_id', default='') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be discovered from the GitHub repository URL and querying https://docs.github.com/en/rest/apps/installations?apiVersion=2022-11-28#list-app-installations-accessible-to-the-user-access-token, and stored somewhere in the DB, not surfaced to the users.
access_token = response.json()['token'] | ||
return access_token | ||
else: | ||
raise Exception(f"Failed to get access token: {response.status_code} {response.text}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance not to use an unspecified exception?
url = f'{github_api_url}/app/installations/{installation_id}/access_tokens' | ||
response = requests.post(url, headers=headers) | ||
|
||
if response.status_code == 201: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you invert it to !=
, you can dedent a few lines of code.
@webknjaz for this implementation we just want to scope this to specifically project update and also this will give the ability for people to have different github app for different projects if desired |
That's kinda against the entire idea of GH Apps 🤷♂️ |
SUMMARY
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION