From c7d04c980bd115e23f8c874a1c2b8d70bd15f872 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 08:34:15 -0700 Subject: [PATCH 01/14] update testing Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 59 +++++++++++-------- pkg/cmd/attestation/api/client_test.go | 40 ++++++++++++- .../attestation/api/mock_httpClient_test.go | 32 +++++++--- 3 files changed, 98 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 37579b7bc..2d491e108 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -176,7 +176,7 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ return fmt.Errorf("attestation has no bundle or bundle URL") } - // for now, we fallback to the bundle field if the bundle URL is empty + // for now, we fall back to the bundle field if the bundle URL is empty if a.BundleURL == "" { c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n") fetched[i] = &Attestation{ @@ -207,35 +207,46 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ func (c *LiveClient) GetBundle(url string) (*bundle.Bundle, error) { c.logger.VerbosePrintf("Fetching attestation bundle with bundle URL\n\n") - resp, err := c.httpClient.Get(url) - if err != nil { - return nil, err - } + var sgBundle *bundle.Bundle + bo := backoff.NewConstantBackOff(getAttestationRetryInterval) + err := backoff.Retry(func() error { + resp, err := c.httpClient.Get(url) + if err != nil { + return backoff.Permanent(err) + } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode) - } + if resp.StatusCode >= 500 && resp.StatusCode <= 599 { + return fmt.Errorf("attestation bundle with URL %s returned status code %d", url, resp.StatusCode) + } - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read blob storage response body: %w", err) - } + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("failed to read blob storage response body: %w", err) + } - var out []byte - decompressed, err := snappy.Decode(out, body) - if err != nil { - return nil, fmt.Errorf("failed to decompress with snappy: %w", err) - } + var out []byte + decompressed, err := snappy.Decode(out, body) + if err != nil { + fmt.Errorf("failed to decompress with snappy: %w", err) + } - var pbBundle v1.Bundle - if err = protojson.Unmarshal(decompressed, &pbBundle); err != nil { - return nil, fmt.Errorf("failed to unmarshal to bundle: %w", err) - } + var pbBundle v1.Bundle + if err = protojson.Unmarshal(decompressed, &pbBundle); err != nil { + return fmt.Errorf("failed to unmarshal to bundle: %w", err) + } - c.logger.VerbosePrintf("Successfully fetched bundle\n\n") + c.logger.VerbosePrintf("Successfully fetched bundle\n\n") - return bundle.NewBundle(&pbBundle) + sgBundle, err = bundle.NewBundle(&pbBundle) + if err != nil { + return fmt.Errorf("failed to create new bundle: %w", err) + } + + return nil + }, backoff.WithMaxRetries(bo, 3)) + + return sgBundle, err } func shouldRetry(err error) bool { diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 65f8d59ca..96edbf061 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -212,7 +212,10 @@ func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { } func TestFetchBundleFromAttestations_Fail(t *testing.T) { - httpClient := &failAfterOneCallHttpClient{} + httpClient := &failAfterNCallsHttpClient{ + FailOnCallN: 2, + FailOnAllSubsequentCalls: true, + } c := &LiveClient{ httpClient: httpClient, @@ -225,7 +228,7 @@ func TestFetchBundleFromAttestations_Fail(t *testing.T) { fetched, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) require.Nil(t, fetched) - httpClient.AssertNumberOfCalls(t, "OnGetFailAfterOneCall", 2) + httpClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) } func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { @@ -244,6 +247,22 @@ func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) } +func TestFetchBundleFromAttestations_FetchByURL5XX_Resp(t *testing.T) { + mockHTTPClient := &failHttpClient{} + + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + + a := makeTestAttestation() + attestations := []*Attestation{&a} + bundle, err := c.fetchBundleFromAttestations(attestations) + require.Error(t, err) + require.Nil(t, bundle) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 3) +} + func TestFetchBundleByURL_FallbackToBundleField(t *testing.T) { mockHTTPClient := &mockHttpClient{} @@ -260,6 +279,23 @@ func TestFetchBundleByURL_FallbackToBundleField(t *testing.T) { mockHTTPClient.AssertNotCalled(t, "OnGetSuccess") } +func TestGetBundle(t *testing.T) { + mockHTTPClient := &failAfterNCallsHttpClient{ + FailOnCallN: 2, + FailOnAllSubsequentCalls: false, + } + + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + + b, err := c.GetBundle("whatever") + require.NoError(t, err) + require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 3) +} + func TestGetTrustDomain(t *testing.T) { fetcher := mockMetaGenerator{ TrustDomain: "foo", diff --git a/pkg/cmd/attestation/api/mock_httpClient_test.go b/pkg/cmd/attestation/api/mock_httpClient_test.go index 4dc34fbf8..074979c2a 100644 --- a/pkg/cmd/attestation/api/mock_httpClient_test.go +++ b/pkg/cmd/attestation/api/mock_httpClient_test.go @@ -40,21 +40,39 @@ func (m *failHttpClient) Get(url string) (*http.Response, error) { }, fmt.Errorf("failed to fetch with %s", url) } -type failAfterOneCallHttpClient struct { +type failHttpClient5XX struct { mock.Mock } -func (m *failAfterOneCallHttpClient) Get(url string) (*http.Response, error) { - m.On("OnGetFailAfterOneCall").Return() +func (m *failHttpClient5XX) Get(url string) (*http.Response, error) { + m.On("OnGetFail").Return() + m.MethodCalled("OnGetFail") - if len(m.Calls) >= 1 { - m.MethodCalled("OnGetFailAfterOneCall") + return &http.Response{ + StatusCode: 500, + }, nil +} + +type failAfterNCallsHttpClient struct { + mock.Mock + FailOnCallN int + FailOnAllSubsequentCalls bool + NumCalls int +} + +func (m *failAfterNCallsHttpClient) Get(url string) (*http.Response, error) { + m.On("OnGetFailAfterNCalls").Return() + + m.NumCalls++ + + if m.NumCalls == m.FailOnCallN || (m.NumCalls > m.FailOnCallN && m.FailOnAllSubsequentCalls) { + m.MethodCalled("OnGetFailAfterNCalls") return &http.Response{ StatusCode: 500, - }, fmt.Errorf("failed to fetch with %s", url) + }, nil } - m.MethodCalled("OnGetFailAfterOneCall") + m.MethodCalled("OnGetFailAfterNCalls") var compressed []byte compressed = snappy.Encode(compressed, data.SigstoreBundleRaw) return &http.Response{ From 4d99ae920c6b056f9077f5e538dd7600738b1795 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 10:34:06 -0700 Subject: [PATCH 02/14] fix tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 60 ++++++++++++++++++-------- 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 96edbf061..9728b265c 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -211,27 +211,22 @@ func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { require.Nil(t, fetched, 2) } -func TestFetchBundleFromAttestations_Fail(t *testing.T) { - httpClient := &failAfterNCallsHttpClient{ - FailOnCallN: 2, - FailOnAllSubsequentCalls: true, - } +func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { + mockHTTPClient := &failHttpClient{} c := &LiveClient{ - httpClient: httpClient, + httpClient: mockHTTPClient, logger: io.NewTestHandler(), } - att1 := makeTestAttestation() - att2 := makeTestAttestation() - attestations := []*Attestation{&att1, &att2} - fetched, err := c.fetchBundleFromAttestations(attestations) + a := makeTestAttestation() + attestations := []*Attestation{&a} + bundle, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) - require.Nil(t, fetched) - httpClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) + require.Nil(t, bundle) } -func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { +func TestFetchBundleFromAttestations_FailWithGetErr(t *testing.T) { mockHTTPClient := &failHttpClient{} c := &LiveClient{ @@ -248,7 +243,10 @@ func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { } func TestFetchBundleFromAttestations_FetchByURL5XX_Resp(t *testing.T) { - mockHTTPClient := &failHttpClient{} + mockHTTPClient := &failAfterNCallsHttpClient{ + FailOnCallN: 1, + FailOnAllSubsequentCalls: true, + } c := &LiveClient{ httpClient: mockHTTPClient, @@ -260,10 +258,9 @@ func TestFetchBundleFromAttestations_FetchByURL5XX_Resp(t *testing.T) { bundle, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) require.Nil(t, bundle) - mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 3) } -func TestFetchBundleByURL_FallbackToBundleField(t *testing.T) { +func TestFetchBundleFromAttestations_FallbackToBundleField(t *testing.T) { mockHTTPClient := &mockHttpClient{} c := &LiveClient{ @@ -280,8 +277,22 @@ func TestFetchBundleByURL_FallbackToBundleField(t *testing.T) { } func TestGetBundle(t *testing.T) { + mockHTTPClient := &mockHttpClient{} + + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + + b, err := c.GetBundle("whatever") + require.NoError(t, err) + require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetSuccess", 1) +} + +func TestGetBundle_Retry(t *testing.T) { mockHTTPClient := &failAfterNCallsHttpClient{ - FailOnCallN: 2, + FailOnCallN: 1, FailOnAllSubsequentCalls: false, } @@ -293,7 +304,20 @@ func TestGetBundle(t *testing.T) { b, err := c.GetBundle("whatever") require.NoError(t, err) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) - mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 3) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) +} + +func TestGetBundle_Fail(t *testing.T) { + mockHTTPClient := &failHttpClient{} + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + + b, err := c.GetBundle("whatever") + require.Error(t, err) + require.Nil(t, b) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) } func TestGetTrustDomain(t *testing.T) { From 40e7353b52a1da047c8ff984a79823239d04a670 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 11:02:33 -0700 Subject: [PATCH 03/14] deduplicate get attestation code Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 2d491e108..526916bcd 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -67,18 +67,9 @@ func (c *LiveClient) BuildRepoAndDigestURL(repo, digest string) string { // GetByRepoAndDigest fetches the attestation by repo and digest func (c *LiveClient) GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) { + c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) url := c.BuildRepoAndDigestURL(repo, digest) - attestations, err := c.getAttestations(url, repo, digest, limit) - if err != nil { - return nil, err - } - - bundles, err := c.fetchBundleFromAttestations(attestations) - if err != nil { - return nil, fmt.Errorf("failed to fetch bundle with URL: %w", err) - } - - return bundles, nil + return c.getByURL(url, limit) } func (c *LiveClient) BuildOwnerAndDigestURL(owner, digest string) string { @@ -88,16 +79,17 @@ func (c *LiveClient) BuildOwnerAndDigestURL(owner, digest string) string { // GetByOwnerAndDigest fetches attestation by owner and digest func (c *LiveClient) GetByOwnerAndDigest(owner, digest string, limit int) ([]*Attestation, error) { + c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) url := c.BuildOwnerAndDigestURL(owner, digest) - attestations, err := c.getAttestations(url, owner, digest, limit) + return c.getByURL(url, limit) +} + +func (c *LiveClient) getByURL(url string, limit int) ([]*Attestation, error) { + attestations, err := c.getAttestations(url, limit) if err != nil { return nil, err } - if len(attestations) == 0 { - return nil, newErrNoAttestations(owner, digest) - } - bundles, err := c.fetchBundleFromAttestations(attestations) if err != nil { return nil, fmt.Errorf("failed to fetch bundle with URL: %w", err) @@ -112,9 +104,7 @@ func (c *LiveClient) GetTrustDomain() (string, error) { return c.getTrustDomain(MetaPath) } -func (c *LiveClient) getAttestations(url, name, digest string, limit int) ([]*Attestation, error) { - c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) - +func (c *LiveClient) getAttestations(url string, limit int) ([]*Attestation, error) { perPage := limit if perPage <= 0 || perPage > maxLimitForFlag { return nil, fmt.Errorf("limit must be greater than 0 and less than or equal to %d", maxLimitForFlag) @@ -157,7 +147,7 @@ func (c *LiveClient) getAttestations(url, name, digest string, limit int) ([]*At } if len(attestations) == 0 { - return nil, newErrNoAttestations(name, digest) + return nil, ErrNoAttestations{} } if len(attestations) > limit { From 5462582401fcc68b3e4a7b13004fec2227715704 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 11:05:17 -0700 Subject: [PATCH 04/14] drop unneeded methods Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 14 ++--------- pkg/cmd/attestation/api/client_test.go | 34 +++++++++++++------------- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 526916bcd..c3670f42d 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -60,27 +60,17 @@ func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClien } } -func (c *LiveClient) BuildRepoAndDigestURL(repo, digest string) string { - repo = strings.Trim(repo, "/") - return fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, repo, digest) -} - // GetByRepoAndDigest fetches the attestation by repo and digest func (c *LiveClient) GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) - url := c.BuildRepoAndDigestURL(repo, digest) + url := fmt.Sprintf(GetAttestationByRepoAndSubjectDigestPath, repo, digest) return c.getByURL(url, limit) } -func (c *LiveClient) BuildOwnerAndDigestURL(owner, digest string) string { - owner = strings.Trim(owner, "/") - return fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, owner, digest) -} - // GetByOwnerAndDigest fetches attestation by owner and digest func (c *LiveClient) GetByOwnerAndDigest(owner, digest string, limit int) ([]*Attestation, error) { c.logger.VerbosePrintf("Fetching attestations for artifact digest %s\n\n", digest) - url := c.BuildOwnerAndDigestURL(owner, digest) + url := fmt.Sprintf(GetAttestationByOwnerAndSubjectDigestPath, owner, digest) return c.getByURL(url, limit) } diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 9728b265c..b93d53b70 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -42,23 +42,23 @@ func NewClientWithMockGHClient(hasNextPage bool) Client { } } -func TestGetURL(t *testing.T) { - c := LiveClient{} - - testData := []struct { - repo string - digest string - expected string - }{ - {repo: "/github/example/", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, - {repo: "/github/example", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, - } - - for _, data := range testData { - s := c.BuildRepoAndDigestURL(data.repo, data.digest) - require.Equal(t, data.expected, s) - } -} +//func TestGetURL(t *testing.T) { +// c := LiveClient{} +// +// testData := []struct { +// repo string +// digest string +// expected string +// }{ +// {repo: "/github/example/", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, +// {repo: "/github/example", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, +// } +// +// for _, data := range testData { +// s := c.BuildRepoAndDigestURL(data.repo, data.digest) +// require.Equal(t, data.expected, s) +// } +//} func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) From fc0d0210c0cfcc37d5f45f8f3a92893690c5b587 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 12:21:27 -0700 Subject: [PATCH 05/14] remove old tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index b93d53b70..77cd22ee6 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -42,24 +42,6 @@ func NewClientWithMockGHClient(hasNextPage bool) Client { } } -//func TestGetURL(t *testing.T) { -// c := LiveClient{} -// -// testData := []struct { -// repo string -// digest string -// expected string -// }{ -// {repo: "/github/example/", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, -// {repo: "/github/example", digest: "sha256:12313213", expected: "repos/github/example/attestations/sha256:12313213"}, -// } -// -// for _, data := range testData { -// s := c.BuildRepoAndDigestURL(data.repo, data.digest) -// require.Equal(t, data.expected, s) -// } -//} - func TestGetByDigest(t *testing.T) { c := NewClientWithMockGHClient(false) attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) From b7f6af03b59f9a697c879a0fda06e44fb134c07b Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 12:42:10 -0700 Subject: [PATCH 06/14] update no attestations found err Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/attestation.go | 16 ++-------------- pkg/cmd/attestation/api/client.go | 2 +- pkg/cmd/attestation/api/client_test.go | 4 ++-- pkg/cmd/attestation/api/mock_httpClient_test.go | 13 ------------- pkg/cmd/attestation/download/download.go | 2 +- pkg/cmd/attestation/download/download_test.go | 2 +- pkg/cmd/attestation/verify/verify.go | 2 +- 7 files changed, 8 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/attestation/api/attestation.go b/pkg/cmd/attestation/api/attestation.go index 16b062a96..fd6b484a7 100644 --- a/pkg/cmd/attestation/api/attestation.go +++ b/pkg/cmd/attestation/api/attestation.go @@ -1,8 +1,7 @@ package api import ( - "fmt" - + "errors" "github.com/sigstore/sigstore-go/pkg/bundle" ) @@ -11,18 +10,7 @@ const ( GetAttestationByOwnerAndSubjectDigestPath = "orgs/%s/attestations/%s" ) -type ErrNoAttestations struct { - name string - digest string -} - -func (e ErrNoAttestations) Error() string { - return fmt.Sprintf("no attestations found for digest %s in %s", e.name, e.digest) -} - -func newErrNoAttestations(name, digest string) ErrNoAttestations { - return ErrNoAttestations{name, digest} -} +var ErrNoAttestationsFound = errors.New("no attestations found") type Attestation struct { Bundle *bundle.Bundle `json:"bundle"` diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index c3670f42d..faa1609fc 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -137,7 +137,7 @@ func (c *LiveClient) getAttestations(url string, limit int) ([]*Attestation, err } if len(attestations) == 0 { - return nil, ErrNoAttestations{} + return nil, ErrNoAttestationsFound } if len(attestations) > limit { diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 77cd22ee6..5a7c299c7 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -132,12 +132,12 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) { attestations, err := c.GetByRepoAndDigest(testRepo, testDigest, DefaultLimit) require.Error(t, err) - require.IsType(t, ErrNoAttestations{}, err) + require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) attestations, err = c.GetByOwnerAndDigest(testOwner, testDigest, DefaultLimit) require.Error(t, err) - require.IsType(t, ErrNoAttestations{}, err) + require.IsType(t, ErrNoAttestationsFound, err) require.Nil(t, attestations) } diff --git a/pkg/cmd/attestation/api/mock_httpClient_test.go b/pkg/cmd/attestation/api/mock_httpClient_test.go index 074979c2a..e11266a60 100644 --- a/pkg/cmd/attestation/api/mock_httpClient_test.go +++ b/pkg/cmd/attestation/api/mock_httpClient_test.go @@ -40,19 +40,6 @@ func (m *failHttpClient) Get(url string) (*http.Response, error) { }, fmt.Errorf("failed to fetch with %s", url) } -type failHttpClient5XX struct { - mock.Mock -} - -func (m *failHttpClient5XX) Get(url string) (*http.Response, error) { - m.On("OnGetFail").Return() - m.MethodCalled("OnGetFail") - - return &http.Response{ - StatusCode: 500, - }, nil -} - type failAfterNCallsHttpClient struct { mock.Mock FailOnCallN int diff --git a/pkg/cmd/attestation/download/download.go b/pkg/cmd/attestation/download/download.go index 7547e9e68..cdbdc0078 100644 --- a/pkg/cmd/attestation/download/download.go +++ b/pkg/cmd/attestation/download/download.go @@ -135,7 +135,7 @@ func runDownload(opts *Options) error { } attestations, err := verification.GetRemoteAttestations(opts.APIClient, params) if err != nil { - if errors.Is(err, api.ErrNoAttestations{}) { + if errors.Is(err, api.ErrNoAttestationsFound) { fmt.Fprintf(opts.Logger.IO.Out, "No attestations found for %s\n", opts.ArtifactPath) return nil } diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index 6c2986065..ddcd08c92 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -276,7 +276,7 @@ func TestRunDownload(t *testing.T) { opts := baseOpts opts.APIClient = api.MockClient{ OnGetByOwnerAndDigest: func(repo, digest string, limit int) ([]*api.Attestation, error) { - return nil, api.ErrNoAttestations{} + return nil, api.ErrNoAttestationsFound }, } diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index 016ec1fa8..a5e78a48e 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -220,7 +220,7 @@ func runVerify(opts *Options) error { attestations, logMsg, err := getAttestations(opts, *artifact) if err != nil { - if ok := errors.Is(err, api.ErrNoAttestations{}); ok { + if ok := errors.Is(err, api.ErrNoAttestationsFound); ok { opts.Logger.Printf(opts.Logger.ColorScheme.Red("✗ No attestations found for subject %s\n"), artifact.DigestWithAlg()) return err } From 611eb86e68d2bd2dc44cd801af11f2ea514ab8c1 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 12:47:25 -0700 Subject: [PATCH 07/14] method update Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 4 ++-- pkg/cmd/attestation/api/client_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index faa1609fc..f6991261c 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -166,7 +166,7 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ } // otherwise fetch the bundle with the provided URL - b, err := c.GetBundle(a.BundleURL) + b, err := c.getBundle(a.BundleURL) if err != nil { return fmt.Errorf("failed to fetch bundle with URL: %w", err) } @@ -184,7 +184,7 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ return fetched, nil } -func (c *LiveClient) GetBundle(url string) (*bundle.Bundle, error) { +func (c *LiveClient) getBundle(url string) (*bundle.Bundle, error) { c.logger.VerbosePrintf("Fetching attestation bundle with bundle URL\n\n") var sgBundle *bundle.Bundle diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 5a7c299c7..f89dcddf2 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -266,7 +266,7 @@ func TestGetBundle(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.GetBundle("whatever") + b, err := c.getBundle("whatever") require.NoError(t, err) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) mockHTTPClient.AssertNumberOfCalls(t, "OnGetSuccess", 1) @@ -283,7 +283,7 @@ func TestGetBundle_Retry(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.GetBundle("whatever") + b, err := c.getBundle("whatever") require.NoError(t, err) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) @@ -296,7 +296,7 @@ func TestGetBundle_Fail(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.GetBundle("whatever") + b, err := c.getBundle("whatever") require.Error(t, err) require.Nil(t, b) mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) From 1d807c22915282d5d576c6406828ef60ec5eb0db Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 13 Jan 2025 12:50:58 -0700 Subject: [PATCH 08/14] add missing return statement Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index f6991261c..359bdad54 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -208,7 +208,7 @@ func (c *LiveClient) getBundle(url string) (*bundle.Bundle, error) { var out []byte decompressed, err := snappy.Decode(out, body) if err != nil { - fmt.Errorf("failed to decompress with snappy: %w", err) + return fmt.Errorf("failed to decompress with snappy: %w", err) } var pbBundle v1.Bundle From 5df2b47d1ff607196f8951e04eaa61cbd6d12a17 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Fri, 24 Jan 2025 09:30:46 -0700 Subject: [PATCH 09/14] update tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index f89dcddf2..f023dd03a 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -283,7 +283,7 @@ func TestGetBundle_Retry(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.getBundle("whatever") + b, err := c.getBundle("mybundleurl") require.NoError(t, err) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) @@ -296,7 +296,7 @@ func TestGetBundle_Fail(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.getBundle("whatever") + b, err := c.getBundle("mybundleurl") require.Error(t, err) require.Nil(t, b) mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) From e9f7761423e3ee24f1debc2621dad92af65ece38 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 30 Jan 2025 09:56:06 -0700 Subject: [PATCH 10/14] dont retry when parsing fails Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 76ef6688a..a950e76eb 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -209,19 +209,19 @@ func (c *LiveClient) getBundle(url string) (*bundle.Bundle, error) { var out []byte decompressed, err := snappy.Decode(out, body) if err != nil { - return fmt.Errorf("failed to decompress with snappy: %w", err) + return backoff.Permanent(fmt.Errorf("failed to decompress with snappy: %w", err)) } var pbBundle v1.Bundle if err = protojson.Unmarshal(decompressed, &pbBundle); err != nil { - return fmt.Errorf("failed to unmarshal to bundle: %w", err) + return backoff.Permanent(fmt.Errorf("failed to unmarshal to bundle: %w", err)) } c.logger.VerbosePrintf("Successfully fetched bundle\n\n") sgBundle, err = bundle.NewBundle(&pbBundle) if err != nil { - return fmt.Errorf("failed to create new bundle: %w", err) + return backoff.Permanent(fmt.Errorf("failed to create new bundle: %w", err)) } return nil From 795263524d1415592588761051b5d9ed97e93bc5 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 30 Jan 2025 11:11:41 -0700 Subject: [PATCH 11/14] change permanent backoff error condition Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index a950e76eb..1e99a2a06 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -193,7 +193,7 @@ func (c *LiveClient) getBundle(url string) (*bundle.Bundle, error) { err := backoff.Retry(func() error { resp, err := c.httpClient.Get(url) if err != nil { - return backoff.Permanent(err) + return fmt.Errorf("request to fetch bundle from URL failed: %w", err) } if resp.StatusCode >= 500 && resp.StatusCode <= 599 { From fc2f18c896605b7274f83a45def22a121a9a7d69 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 30 Jan 2025 11:12:58 -0700 Subject: [PATCH 12/14] consolidate tests around getBundle func when possible Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client_test.go | 102 ++++++++++-------- .../attestation/api/mock_httpClient_test.go | 24 ++++- 2 files changed, 75 insertions(+), 51 deletions(-) diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index cb39fb78a..787408a4e 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -170,10 +170,7 @@ func TestFetchBundleFromAttestations_BundleURL(t *testing.T) { } att1 := makeTestAttestation() - att1.Bundle = nil att2 := makeTestAttestation() - att2.Bundle = nil - // zero out the bundle field so it tries fetching by URL attestations := []*Attestation{&att1, &att2} fetched, err := client.fetchBundleFromAttestations(attestations) require.NoError(t, err) @@ -182,56 +179,28 @@ func TestFetchBundleFromAttestations_BundleURL(t *testing.T) { httpClient.AssertNumberOfCalls(t, "OnGetSuccess", 2) } -func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { +func TestFetchBundleFromAttestations_MissingBundleAndBundleURLFields(t *testing.T) { httpClient := &mockHttpClient{} client := LiveClient{ httpClient: httpClient, logger: io.NewTestHandler(), } + // If both the BundleURL and Bundle fields are empty, the function should + // return an error indicating that att1 := Attestation{} attestations := []*Attestation{&att1} - fetched, err := client.fetchBundleFromAttestations(attestations) - require.Error(t, err) - require.Nil(t, fetched, 2) + bundles, err := client.fetchBundleFromAttestations(attestations) + require.ErrorContains(t, err, "attestation has no bundle or bundle URL") + require.Nil(t, bundles, 2) } -func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { - mockHTTPClient := &failHttpClient{} - // failAfterOneCallHttpClient - - c := &LiveClient{ - httpClient: mockHTTPClient, - logger: io.NewTestHandler(), - } - - a := makeTestAttestation() - attestations := []*Attestation{&a} - bundle, err := c.fetchBundleFromAttestations(attestations) - require.Error(t, err) - require.Nil(t, bundle) -} - -func TestFetchBundleFromAttestations_FailWithGetErr(t *testing.T) { - mockHTTPClient := &failHttpClient{} - - c := &LiveClient{ - httpClient: mockHTTPClient, - logger: io.NewTestHandler(), - } - - a := makeTestAttestation() - a.Bundle = nil - attestations := []*Attestation{&a} - bundle, err := c.fetchBundleFromAttestations(attestations) - require.Error(t, err) - require.Nil(t, bundle) - mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) -} - -func TestFetchBundleFromAttestations_FetchByURL5XX_Resp(t *testing.T) { +func TestFetchBundleFromAttestations_FailOnTheSecondAttestation(t *testing.T) { mockHTTPClient := &failAfterNCallsHttpClient{ - FailOnCallN: 1, + // the initial HTTP request will succeed, which returns a bundle for the first attestation + // all following HTTP requests will fail, which means the function fails to fetch a bundle + // for the second attestation and the function returns an error + FailOnCallN: 2, FailOnAllSubsequentCalls: true, } @@ -240,11 +209,28 @@ func TestFetchBundleFromAttestations_FetchByURL5XX_Resp(t *testing.T) { logger: io.NewTestHandler(), } + att1 := makeTestAttestation() + att2 := makeTestAttestation() + attestations := []*Attestation{&att1, &att2} + bundles, err := c.fetchBundleFromAttestations(attestations) + require.Error(t, err) + require.Nil(t, bundles) +} + +func TestFetchBundleFromAttestations_FailAfterRetrying(t *testing.T) { + mockHTTPClient := &reqFailHttpClient{} + + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + a := makeTestAttestation() attestations := []*Attestation{&a} bundle, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) require.Nil(t, bundle) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetReqFail", 4) } func TestFetchBundleFromAttestations_FallbackToBundleField(t *testing.T) { @@ -255,6 +241,7 @@ func TestFetchBundleFromAttestations_FallbackToBundleField(t *testing.T) { logger: io.NewTestHandler(), } + // If the bundle URL is empty, the code will fallback to the bundle field a := Attestation{Bundle: data.SigstoreBundle(t)} attestations := []*Attestation{&a} fetched, err := c.fetchBundleFromAttestations(attestations) @@ -263,6 +250,7 @@ func TestFetchBundleFromAttestations_FallbackToBundleField(t *testing.T) { mockHTTPClient.AssertNotCalled(t, "OnGetSuccess") } +// getBundle successfully fetches a bundle on the first HTTP request attempt func TestGetBundle(t *testing.T) { mockHTTPClient := &mockHttpClient{} @@ -271,13 +259,15 @@ func TestGetBundle(t *testing.T) { logger: io.NewTestHandler(), } - b, err := c.getBundle("whatever") + b, err := c.getBundle("https://mybundleurl.com") require.NoError(t, err) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", b.GetMediaType()) mockHTTPClient.AssertNumberOfCalls(t, "OnGetSuccess", 1) } -func TestGetBundle_Retry(t *testing.T) { +// getBundle retries successfully when the initial HTTP request returns +// a 5XX status code +func TestGetBundle_SuccessfulRetry(t *testing.T) { mockHTTPClient := &failAfterNCallsHttpClient{ FailOnCallN: 1, FailOnAllSubsequentCalls: false, @@ -294,8 +284,26 @@ func TestGetBundle_Retry(t *testing.T) { mockHTTPClient.AssertNumberOfCalls(t, "OnGetFailAfterNCalls", 2) } -func TestGetBundle_Fail(t *testing.T) { - mockHTTPClient := &failHttpClient{} +// getBundle does not retry when the function fails with a permanent backoff error condition +func TestGetBundle_PermanentBackoffFail(t *testing.T) { + mockHTTPClient := &invalidBundleClient{} + c := &LiveClient{ + httpClient: mockHTTPClient, + logger: io.NewTestHandler(), + } + + b, err := c.getBundle("mybundleurl") + // var permanent *backoff.PermanentError + //require.IsType(t, &backoff.PermanentError{}, err) + require.Error(t, err) + require.Nil(t, b) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetInvalidBundle", 1) +} + +// getBundle retries when the HTTP request fails +func TestGetBundle_RequestFail(t *testing.T) { + mockHTTPClient := &reqFailHttpClient{} + c := &LiveClient{ httpClient: mockHTTPClient, logger: io.NewTestHandler(), @@ -304,7 +312,7 @@ func TestGetBundle_Fail(t *testing.T) { b, err := c.getBundle("mybundleurl") require.Error(t, err) require.Nil(t, b) - mockHTTPClient.AssertNumberOfCalls(t, "OnGetFail", 1) + mockHTTPClient.AssertNumberOfCalls(t, "OnGetReqFail", 4) } func TestGetTrustDomain(t *testing.T) { diff --git a/pkg/cmd/attestation/api/mock_httpClient_test.go b/pkg/cmd/attestation/api/mock_httpClient_test.go index e11266a60..26933ae2e 100644 --- a/pkg/cmd/attestation/api/mock_httpClient_test.go +++ b/pkg/cmd/attestation/api/mock_httpClient_test.go @@ -27,13 +27,29 @@ func (m *mockHttpClient) Get(url string) (*http.Response, error) { }, nil } -type failHttpClient struct { +type invalidBundleClient struct { mock.Mock } -func (m *failHttpClient) Get(url string) (*http.Response, error) { - m.On("OnGetFail").Return() - m.MethodCalled("OnGetFail") +func (m *invalidBundleClient) Get(url string) (*http.Response, error) { + m.On("OnGetInvalidBundle").Return() + m.MethodCalled("OnGetInvalidBundle") + + var compressed []byte + compressed = snappy.Encode(compressed, []byte("invalid bundle bytes")) + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(bytes.NewReader(compressed)), + }, nil +} + +type reqFailHttpClient struct { + mock.Mock +} + +func (m *reqFailHttpClient) Get(url string) (*http.Response, error) { + m.On("OnGetReqFail").Return() + m.MethodCalled("OnGetReqFail") return &http.Response{ StatusCode: 500, From e144168dde109026d0ca4f09a158320245b77ce1 Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Mon, 3 Feb 2025 15:26:05 +0500 Subject: [PATCH 13/14] [gh repo edit] Allow setting commit title defaults --- pkg/cmd/repo/edit/edit.go | 75 +++++++++++++---- pkg/cmd/repo/edit/edit_test.go | 142 ++++++++++++++++++++++++++++++++- 2 files changed, 199 insertions(+), 18 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index daa700cfb..79e32385b 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -70,23 +70,27 @@ type EditRepositoryInput struct { enableSecretScanning *bool enableSecretScanningPushProtection *bool - AllowForking *bool `json:"allow_forking,omitempty"` - AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` - DefaultBranch *string `json:"default_branch,omitempty"` - DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` - Description *string `json:"description,omitempty"` - EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` - EnableIssues *bool `json:"has_issues,omitempty"` - EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` - EnableProjects *bool `json:"has_projects,omitempty"` - EnableDiscussions *bool `json:"has_discussions,omitempty"` - EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` - EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` - EnableWiki *bool `json:"has_wiki,omitempty"` - Homepage *string `json:"homepage,omitempty"` - IsTemplate *bool `json:"is_template,omitempty"` - SecurityAndAnalysis *SecurityAndAnalysisInput `json:"security_and_analysis,omitempty"` - Visibility *string `json:"visibility,omitempty"` + AllowForking *bool `json:"allow_forking,omitempty"` + AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` + DefaultBranch *string `json:"default_branch,omitempty"` + DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` + Description *string `json:"description,omitempty"` + EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` + EnableIssues *bool `json:"has_issues,omitempty"` + EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` + MergeCommitTitle string `json:"merge_commit_title,omitempty"` + MergeCommitMessage string `json:"merge_commit_message,omitempty"` + EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` + SquashMergeCommitTitle string `json:"squash_merge_commit_title,omitempty"` + SquashMergeCommitMessage string `json:"squash_merge_commit_message,omitempty"` + EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` + EnableProjects *bool `json:"has_projects,omitempty"` + EnableDiscussions *bool `json:"has_discussions,omitempty"` + EnableWiki *bool `json:"has_wiki,omitempty"` + Homepage *string `json:"homepage,omitempty"` + IsTemplate *bool `json:"is_template,omitempty"` + SecurityAndAnalysis *SecurityAndAnalysisInput `json:"security_and_analysis,omitempty"` + Visibility *string `json:"visibility,omitempty"` } func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobra.Command { @@ -120,6 +124,22 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr When the %[1]s--visibility%[1]s flag is used, %[1]s--accept-visibility-change-consequences%[1]s flag is required. For information on all the potential consequences, see + + For merge commit (%[1]s--enable-merge-commit%[1]s) and squash merge (%[1]s--enable-squash-merge%[1]s), + following are the valid combinations to set their respective default title and message options: + + - Merge commit (%[1]s--merge-commit-title%[1]s and %[1]s--merge-commit-message%[1]s respectively): + - MERGE_MESSAGE and PR_TITLE (default message) + - PR_TITLE and BLANK (pull request title) + - PR_TITLE and PR_BODY (pull request title and description) + + - Squash merge (%[1]s--squash-merge-commit-title%[1]s and %[1]s--squash-merge-commit-message%[1]s respectively): + - COMMIT_OR_PR_TITLE and COMMIT_MESSAGES (default message) + - PR_TITLE and BLANK (pull request title) + - PR_TITLE and PR_BODY (pull request title and description) + - PR_TITLE and COMMIT_MESSAGES (pull request title and commit details) + + Note: To set the message option, its respective title is required to be set also. `, "`"), Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` @@ -128,6 +148,12 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr # disable projects gh repo edit --enable-projects=false + + # Set merge commit default commit message (pull request title and description) + $ gh repo edit --merge-commit-title=PR_TITLE --merge-commit-message=PR_BODY + + # Set squash merge commit default message (pull request title and commit details) + $ gh repo edit --squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES `), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { @@ -166,6 +192,14 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr opts.Edits.SecurityAndAnalysis = transformSecurityAndAnalysisOpts(opts) } + if opts.Edits.MergeCommitMessage != "" && opts.Edits.MergeCommitTitle == "" { + return cmdutil.FlagErrorf("`--merge-commit-message` must be used in conjunction with `--merge-commit-title`") + } + + if opts.Edits.SquashMergeCommitMessage != "" && opts.Edits.SquashMergeCommitTitle == "" { + return cmdutil.FlagErrorf("`--squash-merge-commit-message` must be used in conjunction with `--squash-merge-commit-title`") + } + if runF != nil { return runF(opts) } @@ -182,8 +216,15 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableProjects, "enable-projects", "", "Enable projects in the repository") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableWiki, "enable-wiki", "", "Enable wiki in the repository") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableDiscussions, "enable-discussions", "", "Enable discussions in the repository") + cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableMergeCommit, "enable-merge-commit", "", "Enable merging pull requests via merge commit") + cmdutil.StringEnumFlag(cmd, &opts.Edits.MergeCommitTitle, "merge-commit-title", "", "", []string{"PR_TITLE", "MERGE_MESSAGE"}, "Default value for a merge commit title") + cmdutil.StringEnumFlag(cmd, &opts.Edits.MergeCommitMessage, "merge-commit-message", "", "", []string{"PR_BODY", "PR_TITLE", "BLANK"}, "Default value for a merge commit message") + cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableSquashMerge, "enable-squash-merge", "", "Enable merging pull requests via squashed commit") + cmdutil.StringEnumFlag(cmd, &opts.Edits.SquashMergeCommitTitle, "squash-merge-commit-title", "", "", []string{"PR_TITLE", "COMMIT_OR_PR_TITLE"}, "Default value for a squash merge commit title") + cmdutil.StringEnumFlag(cmd, &opts.Edits.SquashMergeCommitMessage, "squash-merge-commit-message", "", "", []string{"PR_BODY", "COMMIT_MESSAGES", "BLANK"}, "Default value for a squash merge commit message") + cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableRebaseMerge, "enable-rebase-merge", "", "Enable merging pull requests via rebase") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableAutoMerge, "enable-auto-merge", "", "Enable auto-merge functionality") cmdutil.NilBoolFlag(cmd, &opts.Edits.enableAdvancedSecurity, "enable-advanced-security", "", "Enable advanced security in the repository") diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 868e300fa..ed4aa68c1 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -91,6 +91,145 @@ func TestNewCmdEdit(t *testing.T) { }, }, }, + { + name: "enable merge commit", + args: "--enable-merge-commit=true", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + EnableMergeCommit: bp(true), + }, + }, + }, + { + name: "disable merge commit", + args: "--enable-merge-commit=false", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + EnableMergeCommit: bp(false), + }, + }, + }, + { + name: "set merge commit default commit message (default message)", + args: "--merge-commit-title=MERGE_MESSAGE --merge-commit-message=PR_TITLE", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + MergeCommitTitle: "MERGE_MESSAGE", + MergeCommitMessage: "PR_TITLE", + }, + }, + }, + { + name: "set merge commit default commit message (pull request title)", + args: "--merge-commit-title=PR_TITLE --merge-commit-message=BLANK", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + MergeCommitTitle: "PR_TITLE", + MergeCommitMessage: "BLANK", + }, + }, + }, + { + name: "set merge commit default commit message (pull request title and description)", + args: "--merge-commit-title=PR_TITLE --merge-commit-message=PR_BODY", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + MergeCommitTitle: "PR_TITLE", + MergeCommitMessage: "PR_BODY", + }, + }, + }, + { + name: "set merge commit default commit message (message flag only)", + args: "--merge-commit-message=BLANK", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + MergeCommitMessage: "BLANK", + }, + }, + wantErr: "`--merge-commit-message` must be used in conjunction with `--merge-commit-title`", + }, + { + name: "enable squash merge commit", + args: "--enable-squash-merge=true", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + EnableSquashMerge: bp(true), + }, + }, + }, + { + name: "disable squash merge commit", + args: "--enable-squash-merge=false", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + EnableSquashMerge: bp(false), + }, + }, + }, + { + name: "set squash merge commit default commit message (default message)", + args: "--squash-merge-commit-title=COMMIT_OR_PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SquashMergeCommitTitle: "COMMIT_OR_PR_TITLE", + SquashMergeCommitMessage: "COMMIT_MESSAGES", + }, + }, + }, + { + name: "set squash merge commit default commit message (pull request title)", + args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=BLANK", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SquashMergeCommitTitle: "PR_TITLE", + SquashMergeCommitMessage: "BLANK", + }, + }, + }, + { + name: "set squash merge commit default commit message (pull request title and description)", + args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=PR_BODY", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SquashMergeCommitTitle: "PR_TITLE", + SquashMergeCommitMessage: "PR_BODY", + }, + }, + }, + { + name: "set squash merge commit default commit message (pull request title and commit details)", + args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SquashMergeCommitTitle: "PR_TITLE", + SquashMergeCommitMessage: "COMMIT_MESSAGES", + }, + }, + }, + { + name: "set squash merge commit default commit message (message flag only)", + args: "--squash-merge-commit-message=COMMIT_MESSAGES", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SquashMergeCommitMessage: "COMMIT_MESSAGES", + }, + }, + wantErr: "`--squash-merge-commit-message` must be used in conjunction with `--squash-merge-commit-title`", + }, } for _, tt := range tests { @@ -300,7 +439,8 @@ func Test_editRun_interactive(t *testing.T) { "Template Repository", "Topics", "Visibility", - "Wikis"} + "Wikis", + } tests := []struct { name string From ae20633ba68634c15dc5301a7bdf48e3d59d8a16 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 4 Feb 2025 13:55:37 -0700 Subject: [PATCH 14/14] Revert "[gh repo edit] Allow setting commit message defaults" --- pkg/cmd/repo/edit/edit.go | 75 ++++------------- pkg/cmd/repo/edit/edit_test.go | 142 +-------------------------------- 2 files changed, 18 insertions(+), 199 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 79e32385b..daa700cfb 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -70,27 +70,23 @@ type EditRepositoryInput struct { enableSecretScanning *bool enableSecretScanningPushProtection *bool - AllowForking *bool `json:"allow_forking,omitempty"` - AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` - DefaultBranch *string `json:"default_branch,omitempty"` - DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` - Description *string `json:"description,omitempty"` - EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` - EnableIssues *bool `json:"has_issues,omitempty"` - EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` - MergeCommitTitle string `json:"merge_commit_title,omitempty"` - MergeCommitMessage string `json:"merge_commit_message,omitempty"` - EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` - SquashMergeCommitTitle string `json:"squash_merge_commit_title,omitempty"` - SquashMergeCommitMessage string `json:"squash_merge_commit_message,omitempty"` - EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` - EnableProjects *bool `json:"has_projects,omitempty"` - EnableDiscussions *bool `json:"has_discussions,omitempty"` - EnableWiki *bool `json:"has_wiki,omitempty"` - Homepage *string `json:"homepage,omitempty"` - IsTemplate *bool `json:"is_template,omitempty"` - SecurityAndAnalysis *SecurityAndAnalysisInput `json:"security_and_analysis,omitempty"` - Visibility *string `json:"visibility,omitempty"` + AllowForking *bool `json:"allow_forking,omitempty"` + AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` + DefaultBranch *string `json:"default_branch,omitempty"` + DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` + Description *string `json:"description,omitempty"` + EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` + EnableIssues *bool `json:"has_issues,omitempty"` + EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` + EnableProjects *bool `json:"has_projects,omitempty"` + EnableDiscussions *bool `json:"has_discussions,omitempty"` + EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` + EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` + EnableWiki *bool `json:"has_wiki,omitempty"` + Homepage *string `json:"homepage,omitempty"` + IsTemplate *bool `json:"is_template,omitempty"` + SecurityAndAnalysis *SecurityAndAnalysisInput `json:"security_and_analysis,omitempty"` + Visibility *string `json:"visibility,omitempty"` } func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobra.Command { @@ -124,22 +120,6 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr When the %[1]s--visibility%[1]s flag is used, %[1]s--accept-visibility-change-consequences%[1]s flag is required. For information on all the potential consequences, see - - For merge commit (%[1]s--enable-merge-commit%[1]s) and squash merge (%[1]s--enable-squash-merge%[1]s), - following are the valid combinations to set their respective default title and message options: - - - Merge commit (%[1]s--merge-commit-title%[1]s and %[1]s--merge-commit-message%[1]s respectively): - - MERGE_MESSAGE and PR_TITLE (default message) - - PR_TITLE and BLANK (pull request title) - - PR_TITLE and PR_BODY (pull request title and description) - - - Squash merge (%[1]s--squash-merge-commit-title%[1]s and %[1]s--squash-merge-commit-message%[1]s respectively): - - COMMIT_OR_PR_TITLE and COMMIT_MESSAGES (default message) - - PR_TITLE and BLANK (pull request title) - - PR_TITLE and PR_BODY (pull request title and description) - - PR_TITLE and COMMIT_MESSAGES (pull request title and commit details) - - Note: To set the message option, its respective title is required to be set also. `, "`"), Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` @@ -148,12 +128,6 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr # disable projects gh repo edit --enable-projects=false - - # Set merge commit default commit message (pull request title and description) - $ gh repo edit --merge-commit-title=PR_TITLE --merge-commit-message=PR_BODY - - # Set squash merge commit default message (pull request title and commit details) - $ gh repo edit --squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES `), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { @@ -192,14 +166,6 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr opts.Edits.SecurityAndAnalysis = transformSecurityAndAnalysisOpts(opts) } - if opts.Edits.MergeCommitMessage != "" && opts.Edits.MergeCommitTitle == "" { - return cmdutil.FlagErrorf("`--merge-commit-message` must be used in conjunction with `--merge-commit-title`") - } - - if opts.Edits.SquashMergeCommitMessage != "" && opts.Edits.SquashMergeCommitTitle == "" { - return cmdutil.FlagErrorf("`--squash-merge-commit-message` must be used in conjunction with `--squash-merge-commit-title`") - } - if runF != nil { return runF(opts) } @@ -216,15 +182,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableProjects, "enable-projects", "", "Enable projects in the repository") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableWiki, "enable-wiki", "", "Enable wiki in the repository") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableDiscussions, "enable-discussions", "", "Enable discussions in the repository") - cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableMergeCommit, "enable-merge-commit", "", "Enable merging pull requests via merge commit") - cmdutil.StringEnumFlag(cmd, &opts.Edits.MergeCommitTitle, "merge-commit-title", "", "", []string{"PR_TITLE", "MERGE_MESSAGE"}, "Default value for a merge commit title") - cmdutil.StringEnumFlag(cmd, &opts.Edits.MergeCommitMessage, "merge-commit-message", "", "", []string{"PR_BODY", "PR_TITLE", "BLANK"}, "Default value for a merge commit message") - cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableSquashMerge, "enable-squash-merge", "", "Enable merging pull requests via squashed commit") - cmdutil.StringEnumFlag(cmd, &opts.Edits.SquashMergeCommitTitle, "squash-merge-commit-title", "", "", []string{"PR_TITLE", "COMMIT_OR_PR_TITLE"}, "Default value for a squash merge commit title") - cmdutil.StringEnumFlag(cmd, &opts.Edits.SquashMergeCommitMessage, "squash-merge-commit-message", "", "", []string{"PR_BODY", "COMMIT_MESSAGES", "BLANK"}, "Default value for a squash merge commit message") - cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableRebaseMerge, "enable-rebase-merge", "", "Enable merging pull requests via rebase") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableAutoMerge, "enable-auto-merge", "", "Enable auto-merge functionality") cmdutil.NilBoolFlag(cmd, &opts.Edits.enableAdvancedSecurity, "enable-advanced-security", "", "Enable advanced security in the repository") diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index ed4aa68c1..868e300fa 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -91,145 +91,6 @@ func TestNewCmdEdit(t *testing.T) { }, }, }, - { - name: "enable merge commit", - args: "--enable-merge-commit=true", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - EnableMergeCommit: bp(true), - }, - }, - }, - { - name: "disable merge commit", - args: "--enable-merge-commit=false", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - EnableMergeCommit: bp(false), - }, - }, - }, - { - name: "set merge commit default commit message (default message)", - args: "--merge-commit-title=MERGE_MESSAGE --merge-commit-message=PR_TITLE", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - MergeCommitTitle: "MERGE_MESSAGE", - MergeCommitMessage: "PR_TITLE", - }, - }, - }, - { - name: "set merge commit default commit message (pull request title)", - args: "--merge-commit-title=PR_TITLE --merge-commit-message=BLANK", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - MergeCommitTitle: "PR_TITLE", - MergeCommitMessage: "BLANK", - }, - }, - }, - { - name: "set merge commit default commit message (pull request title and description)", - args: "--merge-commit-title=PR_TITLE --merge-commit-message=PR_BODY", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - MergeCommitTitle: "PR_TITLE", - MergeCommitMessage: "PR_BODY", - }, - }, - }, - { - name: "set merge commit default commit message (message flag only)", - args: "--merge-commit-message=BLANK", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - MergeCommitMessage: "BLANK", - }, - }, - wantErr: "`--merge-commit-message` must be used in conjunction with `--merge-commit-title`", - }, - { - name: "enable squash merge commit", - args: "--enable-squash-merge=true", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - EnableSquashMerge: bp(true), - }, - }, - }, - { - name: "disable squash merge commit", - args: "--enable-squash-merge=false", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - EnableSquashMerge: bp(false), - }, - }, - }, - { - name: "set squash merge commit default commit message (default message)", - args: "--squash-merge-commit-title=COMMIT_OR_PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - SquashMergeCommitTitle: "COMMIT_OR_PR_TITLE", - SquashMergeCommitMessage: "COMMIT_MESSAGES", - }, - }, - }, - { - name: "set squash merge commit default commit message (pull request title)", - args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=BLANK", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - SquashMergeCommitTitle: "PR_TITLE", - SquashMergeCommitMessage: "BLANK", - }, - }, - }, - { - name: "set squash merge commit default commit message (pull request title and description)", - args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=PR_BODY", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - SquashMergeCommitTitle: "PR_TITLE", - SquashMergeCommitMessage: "PR_BODY", - }, - }, - }, - { - name: "set squash merge commit default commit message (pull request title and commit details)", - args: "--squash-merge-commit-title=PR_TITLE --squash-merge-commit-message=COMMIT_MESSAGES", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - SquashMergeCommitTitle: "PR_TITLE", - SquashMergeCommitMessage: "COMMIT_MESSAGES", - }, - }, - }, - { - name: "set squash merge commit default commit message (message flag only)", - args: "--squash-merge-commit-message=COMMIT_MESSAGES", - wantOpts: EditOptions{ - Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), - Edits: EditRepositoryInput{ - SquashMergeCommitMessage: "COMMIT_MESSAGES", - }, - }, - wantErr: "`--squash-merge-commit-message` must be used in conjunction with `--squash-merge-commit-title`", - }, } for _, tt := range tests { @@ -439,8 +300,7 @@ func Test_editRun_interactive(t *testing.T) { "Template Repository", "Topics", "Visibility", - "Wikis", - } + "Wikis"} tests := []struct { name string