Skip to content

nit: Update fly build config #52301

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

Merged
merged 2 commits into from
Jul 6, 2023
Merged

nit: Update fly build config #52301

merged 2 commits into from
Jul 6, 2023

Conversation

nhsiehgit
Copy link
Contributor

cleans up the build_config method

@nhsiehgit nhsiehgit requested review from a team as code owners July 5, 2023 18:33
@nhsiehgit nhsiehgit requested a review from sentaur-athena July 5, 2023 18:33
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 5, 2023
Copy link
Contributor

@EricHasegawa EricHasegawa 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! One question

"""
On configuration, we determine which provider organization to configure sentry SSO for.
This configuration is then stored and passed into the pipeline instances during SSO
to determine whether the Auth'd user has the appropriate access to the provider org
"""
org = organization
if not organization:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is safe to take out? Is it possible for id to not be in resource? And if so, how do we handle that case with the new code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yea

So this isn't being utilized anywhere atm.
Since this is specific per provider, we can shape this build_config however we want per provider.
for Fly in particular, their organization resource will always have an 'id'

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #52301 (c9c3dbf) into master (2d5b88b) will decrease coverage by 2.39%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52301      +/-   ##
==========================================
- Coverage   79.31%   76.92%   -2.39%     
==========================================
  Files        4906     4906              
  Lines      205540   205536       -4     
  Branches    35135    35134       -1     
==========================================
- Hits       163020   158117    -4903     
- Misses      37538    42327    +4789     
- Partials     4982     5092     +110     
Impacted Files Coverage Δ
src/sentry/auth/providers/fly/provider.py 84.84% <100.00%> (+1.06%) ⬆️

... and 342 files with indirect coverage changes

@nhsiehgit nhsiehgit requested a review from EricHasegawa July 6, 2023 17:24
@nhsiehgit nhsiehgit merged commit 2dbcac9 into master Jul 6, 2023
@nhsiehgit nhsiehgit deleted the update_fly_build_config branch July 6, 2023 17:26
@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 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.

2 participants