unknwon

unknwon

Wild kid in the junkyard.

Member Since 9 years ago

@sourcegraph, Shanghai, China

Experience Points
1.19w
follower
Lessons Completed
6
follow
Lessons Completed
8
stars
Best Reply Awards
39
repos

2920 contributions in the last year

Pinned
⚡ Gogs is a painless self-hosted Git service
⚡ Universal code search (self-hosted)
⚡ A fantastic modular Go web framework boiled with dependency injection
⚡ Package macaron is a high productive and modular web framework in Go.
⚡ Package ini provides INI file read and write functionality in Go
⚡ 《The Way to Go》中文译本,中文正式名《Go 入门指南》
Activity
Jan
19
10 hours ago
close pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

unknwon
unknwon

Oops, I think I missed this one, will follow up on this!

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

Activity icon
issue

unknwon issue comment sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

unknwon
unknwon

Thanks everyone! Merging 🎉

push

unknwon push sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App (#29515)

commit sha: 037b501db47c8244619c9dc6fe26c34e647e3962

push time in 34 minutes ago
Activity icon
delete

unknwon in sourcegraph/sourcegraph delete branch jc/CLOUD-204-repositories-syncing-through-github-app

deleted time in 34 minutes ago
pull request

unknwon pull request sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

push

unknwon push sourcegraph/sourcegraph

unknwon
unknwon

One minute timeout for creating access tokens

commit sha: 3859511225a8ec61af72affb91c07a13cf0744b1

push time in 39 minutes ago
Activity icon
issue

unknwon issue comment sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

unknwon
unknwon

@kopancek Please take another look based on comments from Quinn and I!

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

unknwon
unknwon

Jira ticket filed: https://sourcegraph.atlassian.net/browse/CLOUD-233

I've tried to use a search to capture all occurrences of this pattern in the ticket (11 results), and unless we add TODO to all of these places, I don't see much value adding TODO just in this single place. WDYT?

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

unknwon
unknwon

I guess extremely unlikely... but since it could be nil pointer from the compiler's POV, I'd rather be on the safe side 🛡️

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-204] Repositories syncing through GitHub App

This PR adds support of syncing repositories for code host connections created through GitHub App.

One possible optimization is purposely left out with comments to reduce the number of times we create installation access tokens caused by regular page visits.

Demo

https://user-images.githubusercontent.com/2946214/149895449-bad4a729-7106-42ad-b87d-d79ebd0e85cf.mp4

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-204

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

function to validate checker is not nil and is enabled.

When going through past PRs I noticed that I'd missed a check for if the sub-repo permissions checker was nil or not. I figured having a function to always call before using the checker would help catch this type of thing in the future.

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

gqltest: Improve docs on getting integration tests to run locally

unknwon
unknwon

note: if you did wipe out everything including DB, gqltest automatically creates the admin account for you (and performs login in subsequent runs) with supplied credentials.

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

gqltest: Improve docs on getting integration tests to run locally

unknwon
unknwon
commands:
  enterprise-frontend:
    env:
      EXTSVC_CONFIG_FILE: ''
    watch:
      - lib
      - internal
      - cmd/frontend
      - enterprise/internal
      - enterprise/cmd/frontend

Maybe it's a bug in sg, but in my local dev I need the above diff to make auto-rebuild continue to work.

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

gqltest: Improve docs on getting integration tests to run locally

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

gqltest: Improve docs on getting integration tests to run locally

open pull request

unknwon wants to merge go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

unknwon
unknwon

Also, I see codecov is not happy with my patches, is it something I should care about ? (go test -cover gives a different code coverage value from codecov)

Yeah... the difference is annoying, CodeCov is more FYI, don't need to worry much unless diff coverage is 0.

pull request

unknwon merge to go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

pull request

unknwon merge to go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

open pull request

unknwon wants to merge go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

unknwon
unknwon

I do not see what handlings are missing, the value check don't have any meaning since it is in a case where the key has no associated value, and I imagine a boolean key has a value so it wouldn't pass the condition and be handled in the loop like before, am I missing something ?

Ah, you're right. So alternatively, you could add a code commend explaining why we don't need to deal with values here.

But since we can factor out a function, I think having a unified logic with an inline function is relatively more robust (if anything changes to the key name handling).

(partly because I don't really get how the second one would work) Hmm, that was for illustration mostly, on a second look I also forgot how it should work 😅 so forgot about it.

do you really prefer the function to be inline ? If so why ? (Not that I really have anything against it, I am just curious)

Yes, because I don't a reason why it shouldn't be inlined. It is only used here and never meant to be used outside, and the function body is also pretty sophisticated.

Jan
18
1 day ago
open pull request

unknwon wants to merge go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

unknwon
unknwon

I think this block is missing many handlings from where it is copied from (below).

We should either make this loop (starts on line 459) body to be an inline function .e.g writeKey := func(k *Key) error {...}, or do something like:

keys := make([]*Key, 1, len(shadows)+1)
keys[0] = key
keys = append(keys, shadows)
for _, val := range keys {
	...

But the latter is obviously more expensive IMO (due to allocations).

pull request

unknwon merge to go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

pull request

unknwon merge to go-ini/ini

unknwon
unknwon

Return an empty array from `ValueWithShadows` if there is none

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: https://github.com/go-ini/ini/issues/310

Checklist

  • I agree to follow the Code of Conduct by submitting this pull request.
  • I have read and acknowledge the Contributing guide.
  • I have added test cases to cover the new code. There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

open pull request

unknwon wants to merge sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-202] Enforce feature flag check for the GitHub App setup handler

This PR adds a feature flag check for the Sourcegraph Cloud GitHub App setup handler.

An error is displayed for users without having the feature flag:

CleanShot 2022-01-13 at 14 52 25@2x

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-202

unknwon
unknwon

Hmm, sorry, it won't be a "GitHub page not found", but something more complicated as captured in CLOUD-214.

pull request

unknwon merge to sourcegraph/sourcegraph

unknwon
unknwon

[CLOUD-202] Enforce feature flag check for the GitHub App setup handler

This PR adds a feature flag check for the Sourcegraph Cloud GitHub App setup handler.

An error is displayed for users without having the feature flag:

CleanShot 2022-01-13 at 14 52 25@2x

Testing

For testing locally, please follow the local testing guide.


Jira: CLOUD-202

Previous