Modified to use Git with PAT #4534

sZma5a · 2023-07-30T07:24:39Z

What this PR does / why we need it: Modified to use Personal Access Token since currently only SSH can control the Git repository.

Which issue(s) this PR fixes:

Fixes #4106

Does this PR introduce a user-facing change?: Yes

How are users affected by this change: Be able to use Personal Access Token setting like this:

apiVersion: pipecd.dev/v1beta1
kind: Piped
spec:
  git:
     personalAccessToken:
        userName: <user-name>
        userToken: <user-token>

Is this breaking change: No
How to migrate (if breaking change):

sZma5a · 2023-07-30T07:46:27Z

Local behavior testing has not yet been completed...
→Done

hungran · 2023-07-31T02:21:40Z

+1 for this feature

pkg/config/piped.go

kentakozuka

Thank you for your PR!
Left comments. PTAL 👀
Also, please review the test data and make the test pass 👍

kentakozuka · 2023-08-01T23:50:53Z

pkg/config/piped.go

@@ -176,6 +179,7 @@ func (s *PipedSpec) Mask() {
 		s.PipedKeyData = maskString
 	}
 	s.Git.Mask()
+	s.Git.PersonalAccessToken.Mask()


IMHO, it would be better to move this line to s.Git.Mask().

kentakozuka · 2023-08-02T00:31:24Z

pkg/config/piped.go

+	if g.ShouldConfigureSSHConfig() && g.PersonalAccessToken.ShouldConfigureSSHConfig() {
+		return errors.New("cannot configure both sshKeyData or sshKeyFile and personalAccessToken")
+	}
+	return nil


Please try not to use ShouldConfigureSSHConfig? This function is supposed to be used in a different context. Validate configs here.

@kentakozuka
Sorry... I forgot to fix name when I copied.
How about "ShouldConfigurePATConfig" because the function is checking to use PAT or not?

kentakozuka · 2023-08-02T00:32:10Z

pkg/config/piped.go

+	UserToken string `json:"userToken,omitempty"`
+}
+
+func (p PipedGitPersonalAccessToken) ShouldConfigureSSHConfig() bool {


Suggested change

func (p PipedGitPersonalAccessToken) ShouldConfigureSSHConfig() bool {

func (p PipedGitPersonalAccessToken) Validate() bool {

sivchari

some nits.

sivchari · 2023-08-02T15:09:10Z

pkg/config/piped_test.go

@@ -1289,3 +1306,46 @@ func TestFindPlatformProvidersByLabel(t *testing.T) {
 		})
 	}
 }
+
+func TestPipeGitValidate(t *testing.T) {
+	testcases := []struct {


Suggested change

testcases := []struct {

t.Parallel()

testcases := []struct {

sivchari · 2023-08-02T15:09:39Z

pkg/config/piped_test.go

+	for _, tc := range testcases {
+		t.Run(tc.git.SSHKeyData, func(t *testing.T) {
+			err := tc.git.Validate()


Suggested change

for _, tc := range testcases {

t.Run(tc.git.SSHKeyData, func(t *testing.T) {

err := tc.git.Validate()

for _, tc := range testcases {

tt := tt

t.Run(tc.git.SSHKeyData, func(t *testing.T) {

t.Parallel()

err := tc.git.Validate()

sivchari · 2023-08-02T15:10:13Z

pkg/config/piped_test.go

+func TestPipeGitValidate(t *testing.T) {
+	testcases := []struct {
+		git           PipedGit
+		expectedError error


Suggested change

expectedError error

err error

sivchari · 2023-08-02T15:12:31Z

pkg/config/piped_test.go

+			if tc.expectedError != nil {
+				assert.Equal(t, tc.expectedError, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})


Suggested change

if tc.expectedError != nil {

assert.Equal(t, tc.expectedError, err)

} else {

require.NoError(t, err)

}

})

assert.Equal(t, tc.err, err)

})

kentakozuka

Thanks!
Can you run make test/go to make sure all tests pass?
Also, docs needs to be updated.

kentakozuka · 2023-08-03T01:27:04Z

pkg/config/piped.go

@@ -345,6 +351,13 @@ func (g PipedGit) LoadSSHKey() ([]byte, error) {
 	return nil, errors.New("either sshKeyFile or sshKeyData must be set")
 }

+func (g *PipedGit) Validate() error {
+	if g.ShouldConfigureSSHConfig() && g.PersonalAccessToken.ShouldConfigurePATConfig() {


ShouldConfigureSSHConfig() is not exactly for validation purpose(called here). Can you validate ssh and pat config without using ShouldConfigureXXX()?

sZma5a · 2023-08-13T04:01:55Z

Sorry for the delay.
I have fixed the points you reviewed.

sZma5a · 2023-08-14T09:55:10Z

Sorry, I just checked at hand and the test was down, so please hold on a moment for a review.

codecov · 2023-08-18T06:10:35Z

Codecov Report

Patch coverage: 27.35% and project coverage change: +0.01% 🎉

Comparison is base (41d8b92) 29.98% compared to head (a798b9a) 29.99%.
Report is 17 commits behind head on master.

❗ Current head a798b9a differs from pull request most recent head 01ad346. Consider uploading reports for the commit 01ad346 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4534      +/-   ##
==========================================
+ Coverage   29.98%   29.99%   +0.01%     
==========================================
  Files         220      220              
  Lines       25808    25909     +101     
==========================================
+ Hits         7739     7772      +33     
- Misses      17422    17487      +65     
- Partials      647      650       +3

Files Changed	Coverage Δ
pkg/app/piped/notifier/notifier.go	`0.00% <0.00%> (ø)`
pkg/app/piped/notifier/slack.go	`3.57% <0.00%> (-0.25%)`	⬇️
pkg/app/piped/platformprovider/ecs/client.go	`0.00% <0.00%> (ø)`
pkg/app/piped/platformprovider/ecs/ecs.go	`0.00% <ø> (ø)`
pkg/app/piped/platformprovider/lambda/client.go	`4.73% <0.00%> (-0.21%)`	⬇️
pkg/app/piped/platformprovider/lambda/function.go	`63.75% <ø> (ø)`
pkg/git/client.go	`46.87% <42.85%> (-0.80%)`	⬇️
pkg/config/piped.go	`57.33% <88.46%> (+1.31%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sZma5a · 2023-08-24T14:23:53Z

I had a lint error in Github Actions, so I tried again in my local enviroment, but it appears that some of these are outside the scope of this PR, how should I handle this?

pkg/config/application.go:255:2: ST1003: struct field Id should be ID (stylecheck)
        Id      string          `json:"id"`
        ^
pkg/config/application.go:443:2: ST1003: struct field Https should be HTTPS (stylecheck)
        Https            []TemplatableAnalysisHTTP    `json:"https"`
        ^
pkg/config/control_plane.go:55:2: ST1003: struct field Id should be ID (stylecheck)
        Id string `json:"id"`

kentakozuka

Thanks!
Left some comments.
CI is passing, so ignore the lint error on local for now.
(I also ran make lint/go on local and got a bunch of errors. We should fix them in another PR 😉 )

kentakozuka · 2023-08-24T23:59:53Z

pkg/config/piped.go

+func (p PipedGitPersonalAccessToken) Validate() bool {
+	return p.UserName != "" && p.UserToken != ""


Can you make this function return error for consistency?
ref: https://github.com/pipe-cd/pipecd/blob/master/pkg/config/piped.go#L103

kentakozuka · 2023-08-25T00:07:05Z

pkg/config/piped.go

+func (g *PipedGit) Validate() error {
+	if g.ShouldConfigureSSHConfig() && g.PersonalAccessToken.Validate() {
+		return errors.New("cannot configure both sshKeyData or sshKeyFile and personalAccessToken")
+	}
+	return nil
+}


Can you add a unit test for this function?
I don't feel this is not enough. What about a case in which one of ShouldConfigureSSHConfig and PersonalAccessToken.Validate returns false?

Corrected missing test case!

kentakozuka · 2023-08-25T00:07:22Z

docs/content/en/docs-dev/user-guide/managing-piped/configuration-reference.md

+## GitPersonalAccessToken
+| Field | Type | Description | Required |
+|-|-|-|-|
+| userName | string | The user name for git  used while cloning above Git repository. | No |


kentakozuka · 2023-08-25T00:07:30Z

docs/content/en/docs-dev/user-guide/managing-piped/configuration-reference.md

+| Field | Type | Description | Required |
+|-|-|-|-|
+| userName | string | The user name for git  used while cloning above Git repository. | No |
+| userToken | string | The generated personal access token used while cloning above Git repository. | No |


github-actions bot added the area/go label Jul 30, 2023

sZma5a force-pushed the feat/add-pat-setting branch from 26f8907 to 634ff91 Compare July 30, 2023 07:25

sZma5a changed the title ~~Modified to use Git with PAT~~ [WIP]Modified to use Git with PAT Jul 30, 2023

sivchari reviewed Jul 31, 2023

View reviewed changes

pkg/config/piped.go Outdated Show resolved Hide resolved

pkg/config/piped.go Outdated Show resolved Hide resolved

sZma5a force-pushed the feat/add-pat-setting branch from 89e8a20 to eb372f7 Compare August 1, 2023 10:51

sZma5a changed the title ~~[WIP]Modified to use Git with PAT~~ Modified to use Git with PAT Aug 1, 2023

sZma5a marked this pull request as ready for review August 1, 2023 14:53

sZma5a requested review from nghialv, khanhtc1202 and kentakozuka as code owners August 1, 2023 14:53

kentakozuka reviewed Aug 2, 2023

View reviewed changes

sZma5a force-pushed the feat/add-pat-setting branch 3 times, most recently from 44bfbae to fc0e001 Compare August 2, 2023 13:29

sivchari reviewed Aug 2, 2023

View reviewed changes

kentakozuka reviewed Aug 3, 2023

View reviewed changes

sZma5a force-pushed the feat/add-pat-setting branch from aa88e0e to 3d4abae Compare August 13, 2023 03:57

sZma5a requested review from kentakozuka and sivchari August 14, 2023 09:40

kentakozuka reviewed Aug 25, 2023

View reviewed changes

sZma5a force-pushed the feat/add-pat-setting branch from 7f6a679 to 01ad346 Compare August 27, 2023 16:18

sZma5a closed this Aug 27, 2023

sZma5a force-pushed the feat/add-pat-setting branch from 01ad346 to 78cfc03 Compare August 27, 2023 16:20

sZma5a mentioned this pull request Aug 27, 2023

Modified to use Git with PAT #4571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified to use Git with PAT #4534

Modified to use Git with PAT #4534

sZma5a commented Jul 30, 2023 •

edited

Loading

sZma5a commented Jul 30, 2023 •

edited

Loading

hungran commented Jul 31, 2023

kentakozuka left a comment

kentakozuka Aug 1, 2023

kentakozuka Aug 2, 2023

sZma5a Aug 2, 2023

kentakozuka Aug 2, 2023

sivchari left a comment

sivchari Aug 2, 2023

sivchari Aug 2, 2023

sivchari Aug 2, 2023

sivchari Aug 2, 2023

kentakozuka left a comment

kentakozuka Aug 3, 2023

sZma5a commented Aug 13, 2023

sZma5a commented Aug 14, 2023

codecov bot commented Aug 18, 2023 •

edited

Loading

sZma5a commented Aug 24, 2023

kentakozuka left a comment

kentakozuka Aug 24, 2023

sZma5a Aug 27, 2023

kentakozuka Aug 25, 2023

sZma5a Aug 27, 2023

kentakozuka Aug 25, 2023

kentakozuka Aug 25, 2023

	func (p PipedGitPersonalAccessToken) ShouldConfigureSSHConfig() bool {
	func (p PipedGitPersonalAccessToken) Validate() bool {

		func (p PipedGitPersonalAccessToken) Validate() bool {
		return p.UserName != "" && p.UserToken != ""

	\| userName \| string \| The user name for git used while cloning above Git repository. \| No \|
	\| userName \| string \| The user name for git used while cloning above Git repository. \| Yes \|

	\| userToken \| string \| The generated personal access token used while cloning above Git repository. \| No \|
	\| userToken \| string \| The generated personal access token used while cloning above Git repository. \| Yes \|

Modified to use Git with PAT #4534

Modified to use Git with PAT #4534

Conversation

sZma5a commented Jul 30, 2023 • edited Loading

sZma5a commented Jul 30, 2023 • edited Loading

hungran commented Jul 31, 2023

kentakozuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivchari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentakozuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sZma5a commented Aug 13, 2023

sZma5a commented Aug 14, 2023

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

sZma5a commented Aug 24, 2023

kentakozuka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sZma5a commented Jul 30, 2023 •

edited

Loading

sZma5a commented Jul 30, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading