feat: initial Github application #1
Conversation
There was a problem hiding this comment.
it would be nice to keep these files for consistency with all other projects - unless they're not needed? 🤔
| if not integration.enabled: | ||
| continue | ||
|
|
||
| GITHUB_TOKEN = _get_token(integration.app_id, integration.secrets, integration.app_installation_id) |
There was a problem hiding this comment.
Not a constant anyway :P If it was like a single-time fetched result, it would be ok - but this is not as much.
| GITHUB_TOKEN = _get_token(integration.app_id, integration.secrets, integration.app_installation_id) | |
| github_token = _get_token(integration.app_id, integration.secrets, integration.app_installation_id) |
| self._parse_users(integration) | ||
| self._parse_teams(integration) | ||
|
|
||
| def _parse_users(self, integration): |
There was a problem hiding this comment.
Is there any advantage of using GitHub's GraphQL instead of the regular API? I know, the answer is probably tidier but this type of queries are a bit cumbersome to read 🙃
| variables = {'organisation': integration.organisation} | ||
|
|
||
| response = requests.post( | ||
| 'https://api.github.com/graphql', json={'query': query, 'variables': variables}, headers=self.headers |
There was a problem hiding this comment.
api.github.com might be a good candidate for a constant variable somewhere in this script :)
| 'https://api.github.com/graphql', json={'query': query, 'variables': variables}, headers=self.headers | ||
| ) | ||
|
|
||
| if response.status_code == 200: |
There was a problem hiding this comment.
Instead of this if/else, try response.raise_for_status() and then to the body parsing. It's much cleaner and built-in in the requests lib.
|
|
||
| class GithubTeam(models.Model): | ||
| id = models.CharField(max_length=128, null=False, primary_key=True, editable=False) | ||
| name = models.CharField(max_length=128, null=False, editable=False) |
There was a problem hiding this comment.
null=False is the default, no need to write it there.
| number = models.IntegerField(editable=False) | ||
| repository = models.ForeignKey('inventory.GitRepository', on_delete=models.CASCADE, editable=False) | ||
| url = models.URLField(null=True, blank=True, editable=False) | ||
| secret_type = models.CharField(max_length=255, null=True, blank=True, editable=False) |
There was a problem hiding this comment.
Would this be a good candidate for a enumerator? Type suggests that.
| @@ -0,0 +1,3 @@ | |||
| from django.test import TestCase | |||
|
|
|||
| # Create your tests here. | |||
There was a problem hiding this comment.
It would be great if you could write some, not gonna lie 🙃
| return f'{self.pk} [{self.cached_content_source.app_label}] - {self.name}' | ||
|
|
||
|
|
||
| class Application(models.Model): |
There was a problem hiding this comment.
A suggestion could be to use Surface as a lib instead of copy/pasting these fields.
| pytest==6.2.5 | ||
| pytest-cov==2.12.1 | ||
| pytest-django==4.4.0 |
There was a problem hiding this comment.
These versions should be bumped to the latest.
No description provided.