Do not source system- and user- Git settings / Fix GitMirrorAuthTest
#816
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: We can't test Git-over-HTTPS authentication with `GitMirrorAuthTest` because of the following: - JGit silently sources the custom configurations, breaking our expected behaviors: - `~/.gitconfig` - `~/.config/jgit` - `~/.jgitconfig` - `/etc/gitconfig` - It's not convenient to pass Git username and password via system properties. Modifications: - Added `IsolatedSystemReader` that prevents reading the system- and user- Git settings so that Central Dogma doesn't behave differently between environments. - Revamped `GitMirrorAuthTest` so that it always uses `github.com/line/centraldogma-authtest.git` as its test repository. - The Git-over-SSH test now always runs as a part of the build. - The Git-over-HTTPS test requires sensitive information, so it still has to run manually, although we could at least run it as a part of GitHub action (but not in a PR build). - Updated `GitMirrorAuthTest` so that it pulls GitHub username and password via environment variables rather than system properties. Result: - Central Dogma doesn't behave differently between environments. - `GitMirrorAuthTest` now always runs the Git-over-SSH test. - `GitMirrorAuthTest` now uses environment variables rather than JVM system properties to retrieve GitHub username and password.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #816 +/- ##
============================================
+ Coverage 65.50% 65.59% +0.08%
- Complexity 3294 3308 +14
============================================
Files 353 354 +1
Lines 13796 13816 +20
Branches 1492 1492
============================================
+ Hits 9037 9062 +25
+ Misses 3911 3908 -3
+ Partials 848 846 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
jrhee17
approved these changes
Mar 3, 2023
| */ | ||
| private static final String GIT_PASSPHRASE = System.getProperty("git.passphrase"); | ||
| private static final String GITHUB_PASSWORD = | ||
| System.getenv("GITHUB_CD_AUTHTEST_PASSWORD"); |
There was a problem hiding this comment.
As-is is also fine, but I'm wondering why we don't fallback to user.password for symmetry
Suggested change
| System.getenv("GITHUB_CD_AUTHTEST_PASSWORD"); | |
| System.getenv("GITHUB_CD_AUTHTEST_PASSWORD"), | |
| System.getProperty("user.password")) |
There was a problem hiding this comment.
user.name is JDK standard property which is always given, whereas user.password is not. 😉 Let me leave it as it is.
| @@ -0,0 +1,20 @@ | |||
| -----BEGIN EC PRIVATE KEY----- | |||
There was a problem hiding this comment.
Question) Just to make sure, is this a read-only deploy key?
ikhoon
approved these changes
Mar 3, 2023
thachlp
reviewed
Mar 7, 2023
server/src/main/java/com/linecorp/centraldogma/server/internal/IsolatedSystemReader.java
Show resolved
Hide resolved
…/IsolatedSystemReader.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
We can't test Git-over-HTTPS authentication with
GitMirrorAuthTestbecause of the following:~/.gitconfig~/.config/jgit~/.jgitconfig/etc/gitconfigModifications:
IsolatedSystemReaderthat prevents reading the system- and user- Git settings so that Central Dogma doesn't behave differently between environments.GitMirrorAuthTestso that it always usesgithub.com/line/centraldogma-authtest.gitas its test repository.GitMirrorAuthTestso that it pulls GitHub username and password via environment variables rather than system properties.Result:
GitMirrorAuthTestnow always runs the Git-over-SSH test.GitMirrorAuthTestnow uses environment variables rather than JVM system properties to retrieve GitHub username and password.