Skip to content

GitHub App: track extra metadata#12829

Open
stsewd wants to merge 3 commits intomainfrom
gh-installation-metadata
Open

GitHub App: track extra metadata#12829
stsewd wants to merge 3 commits intomainfrom
gh-installation-metadata

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 4, 2026

We could also make these extra properties that dynamically get the information from extra_data as well (they are present in all objects). Since we weren't updating the extra data field after the integration was created we will need to update all of them (we have less than 2K installations).

from readthedocs.oauth.clients import get_gh_app_client
client = get_gh_app_client()
for installation in GitHubAppInstallation.objects.iterator():
    gh_installation = client.get_app_installation(installation.installation_id)
    gh_installation.extra_data = {"installation": gh_installation.raw_data}
    gh_installation.save()

Closes https://github.com/readthedocs/readthedocs-corporate/issues/2046

@stsewd stsewd requested a review from a team as a code owner March 4, 2026 23:23
@stsewd stsewd requested a review from humitos March 4, 2026 23:23
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

    gh_installation.extra_data = {"installation": gh_installation.raw_data}

Why are we saving this data under a installation key, the object is the installation already, there is no need to have a nested field inside it. We should just do gh_installation.extra_data = gh_installation.raw_data for simplification.

Comment on lines +93 to +98
target_login = models.CharField(
help_text=_("The account login the installation belongs to"),
default="",
null=True,
max_length=255,
)
Copy link
Member

Choose a reason for hiding this comment

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

What are the options for this field? user and organization only?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the login of the user/organization.

Copy link
Member

Choose a reason for hiding this comment

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

I don't follow you.

Copy link
Member Author

Choose a reason for hiding this comment

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

the username of the user/organization.

Comment on lines +95 to +96
default="",
null=True,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
default="",
null=True,
null=True,
blank=True,

Remove the default="" so it uses None by default. Also add blank so we can define it from the admin.

Copy link
Member Author

Choose a reason for hiding this comment

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

All objects in production will have this field populated after the data migration.

@stsewd
Copy link
Member Author

stsewd commented Mar 5, 2026

Why are we saving this data under a installation key, the object is the installation already, there is no need to have a nested field inside it. We should just do gh_installation.extra_data = gh_installation.raw_data for simplification.

Extra data can contain more info related to event when the installation was created.

@humitos
Copy link
Member

humitos commented Mar 9, 2026

Extra data can contain more info related to event when the installation was created.

In that case, we should call gh_installation.extra_data.update(extra_data) to avoid overriding other fields in the JSON, shouldn't we?

@stsewd
Copy link
Member Author

stsewd commented Mar 10, 2026

In that case, we should call gh_installation.extra_data.update(extra_data) to avoid overriding other fields in the JSON, shouldn't we?

During the migration yeah, after that it's fine to override.

@humitos
Copy link
Member

humitos commented Mar 10, 2026

In that case, after the migration we won't have other fields inside the JSON... that goes back to my original question? Why are we saving it as = {"installation": data} instead of just = data?

@stsewd
Copy link
Member Author

stsewd commented Mar 10, 2026

In that case, after the migration we won't have other fields inside the JSON.

After the migration we will still have extra fields in the data field, that's from when the installation was updated

data = self.data.copy()

At the end, this is just extra metadata to debug, or make it easy to inspect some fields without re-fetching the installation.

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.

2 participants