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

fix(apis): commit auto generated code #56698

Merged
merged 12 commits into from
Oct 11, 2023
Merged

Conversation

sentaur-athena
Copy link
Member

@sentaur-athena sentaur-athena commented Sep 21, 2023

make build-api-docs makes some file changes now, before this change the changes wouldn't get added to the commit in the PR. This new workflow adds them to the PR.

@@ -47,13 +47,27 @@ jobs:
if: steps.changes.outputs.api_docs == 'true'

- name: Build OpenAPI Derefed JSON
if: steps.changes.outputs.api_docs == 'true'
# if: steps.changes.outputs.api_docs == 'true'
Copy link
Member Author

Choose a reason for hiding this comment

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

commented temporarily to make sure build-api-docs runs every time

@@ -653,6 +653,7 @@
"OrganizationStatsEndpointV2::GET"
],
"private": [
"NotificationActionsIndexEndpoint::GET",
Copy link
Member Author

Choose a reason for hiding this comment

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

These are the changes by the workflow and looks right.

private_key: ${{ secrets.SENTRY_INTERNAL_APP_PRIVATE_KEY }}

- name: Apply API Publish Status or Ownership Changes
# if: env.SECRET_ACCESS == 'true' && startsWith(github.ref, 'refs/pull') && always()
Copy link
Member Author

Choose a reason for hiding this comment

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

Do I need this line? The step is not called when this is not commented now 😛

@sentaur-athena sentaur-athena marked this pull request as ready for review September 22, 2023 18:59
@sentaur-athena sentaur-athena requested a review from a team as a code owner September 22, 2023 18:59
Copy link
Member

@asottile-sentry asottile-sentry left a comment

Choose a reason for hiding this comment

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

we shouldn't be checking in build outputs in the first place -- why are these managed by git?

@sentaur-athena
Copy link
Member Author

@asottile-sentry

we shouldn't be checking in build outputs in the first place -- why are these managed by git?

I'm building a bunch of accountability measures for apis to know what teams own what and what apis are reviewed and published vs. not. make build-api-docs is where these are being checked and the build fails for example if someone tries to add an api without ownership.

I figured it's also the best place to keep the ownership file updated. If not with git workflow, what's another way that I can autogenerate code and push with people's PRs?

@mikejihbe
Copy link
Contributor

Can we just make this a pre-commit check instead of doing this in github actions?

@sentaur-athena
Copy link
Member Author

@mikejihbe I believe we can. We can also add it to the openapi.yml which is already a github action that commits to a different repo. I don't mind having the ownership file stored in a different repo.

@asottile-sentry what do you think?

@asottile-sentry
Copy link
Member

yeah a separate repo is better -- still should ideally be an actual datastore but it won't impact sentry devs unless they are cloning that other repo 👍

push:
branches:
- master
pull_request:
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'm not going to merge this change, only here so we can see the action on this PR.

@@ -28,14 +28,15 @@
"SourceMapDebugEndpoint::GET"
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 file won't be here if they don't run make build-api-docs locally.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, I put it in git ignore so people don't even see it.

@@ -10,7 +10,7 @@
@control_silo_endpoint
class NotificationDefaultsEndpoints(Endpoint):
publish_status = {
"GET": ApiPublishStatus.PRIVATE,
"GET": ApiPublishStatus.EXPERIMENTAL,
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 will not merge this change. This is only to show these changes will be applied to the other repo. They are: https://github.com/getsentry/sentry-api-schema/blob/main/api_ownership_stats_dont_modify.json#L37

@sentaur-athena sentaur-athena marked this pull request as ready for review October 10, 2023 20:30
@sentaur-athena sentaur-athena enabled auto-merge (squash) October 10, 2023 22:40
@sentaur-athena sentaur-athena merged commit c9f5629 into master Oct 11, 2023
46 checks passed
@sentaur-athena sentaur-athena deleted the athena/github-action branch October 11, 2023 12:36
@github-actions github-actions bot locked and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants