Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Reintroduce ordering of auth providers#50586

Merged
kopancek merged 4 commits into
mainfrom
milan/order_login_items_again
Apr 13, 2023
Merged

Reintroduce ordering of auth providers#50586
kopancek merged 4 commits into
mainfrom
milan/order_login_items_again

Conversation

@kopancek

@kopancek kopancek commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Description

This PR reintroduces the changes that were reverted in

Also fixes a problem where I did not handle correctly the case when certain fields have zero value.

Test plan

@cla-bot cla-bot Bot added the cla-signed label Apr 13, 2023
@kopancek kopancek requested a review from a team April 13, 2023 10:53
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Apr 13, 2023

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.02% (+2.30 kb) 0.02% (+2.30 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 4250b89 and c9ddb0d or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@vdavid vdavid left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! I only looked at the second commit (the fix) as the rest was already reviewed. Makes sense, and thanks for the thorough testing!

@sashaostrikov sashaostrikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@kopancek

Copy link
Copy Markdown
Contributor Author

Previous main-dry-run failed, so I added some more changes and more unit tests to make sure this time it will succeed.

@willdollman

Copy link
Copy Markdown
Contributor

This is one of a few PRs where the SonarCloud Code Analysis check is stuck on "Expected — Waiting for status to be reported". Seems to be a problem on either the github or sonarcloud end as the check passed.

Hopefully it's a transient issue and will fix itself if the checks are rerun (pushing a new commit or rerunning them manually), but let me know if not.

@kopancek kopancek force-pushed the milan/order_login_items_again branch from 92c01c7 to 4250b89 Compare April 13, 2023 17:09
@kopancek

Copy link
Copy Markdown
Contributor Author

Force pushed a small change to the last commit message to see if it would help

@kopancek kopancek merged commit c5f558c into main Apr 13, 2023
@kopancek kopancek deleted the milan/order_login_items_again branch April 13, 2023 17:37
almeidapaulooliveira pushed a commit that referenced this pull request Apr 13, 2023
## Description

This PR reintroduces the changes that were reverted in 
- #50450

Also fixes a problem where I did not handle correctly the case when
certain fields have zero value.

## Test plan

- [x] New unit test added for the fixed changes.
- [x] [main-dry-run can be seen
here](https://buildkite.com/sourcegraph/sourcegraph/builds/213015)
cesrjimenez pushed a commit that referenced this pull request Apr 14, 2023
## Description

This PR reintroduces the changes that were reverted in 
- #50450

Also fixes a problem where I did not handle correctly the case when
certain fields have zero value.

## Test plan

- [x] New unit test added for the fixed changes.
- [x] [main-dry-run can be seen
here](https://buildkite.com/sourcegraph/sourcegraph/builds/213015)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants