From da528ddac7fa1a9596c09302c705e174f326ad63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 15:10:38 +0300 Subject: [PATCH 01/19] Remove now unnecessary doc.go --- github/doc.go | 21 --------------------- 1 file changed, 21 deletions(-) delete mode 100644 github/doc.go diff --git a/github/doc.go b/github/doc.go deleted file mode 100644 index 088bc948..00000000 --- a/github/doc.go +++ /dev/null @@ -1,21 +0,0 @@ -/* -Copyright 2020 The Flux CD contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package github - -import ( - _ "github.com/google/go-github/v32/github" -) From b16591cb0888a773961c747a2cfa8561269c7e73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 16:15:32 +0300 Subject: [PATCH 02/19] Diff state effectively for repository/deploykey reconciliation --- github/integration_test.go | 13 ++++---- github/resource_deploykey.go | 28 ++++++++++++++++- github/resource_repository.go | 57 +++++++++++++++++++++++++++++++---- 3 files changed, 84 insertions(+), 14 deletions(-) diff --git a/github/integration_test.go b/github/integration_test.go index 7f4ed204..6377c54c 100644 --- a/github/integration_test.go +++ b/github/integration_test.go @@ -26,6 +26,7 @@ import ( "testing" "time" + "github.com/google/go-github/v32/github" githubapi "github.com/google/go-github/v32/github" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -156,8 +157,10 @@ var _ = Describe("GitHub Provider", func() { getRepo, err := c.OrgRepositories().Get(ctx, repoRef) Expect(err).ToNot(HaveOccurred()) - // Expect the two responses (one from POST and one from GET to be equal) - Expect(getRepo.Get()).To(Equal(repo.Get())) + // Expect the two responses (one from POST and one from GET to have equal "spec") + getSpec := newGithubRepositorySpec(getRepo.APIObject().(*github.Repository)) + postSpec := newGithubRepositorySpec(repo.APIObject().(*github.Repository)) + Expect(getSpec.Equals(postSpec)).To(BeTrue()) }) It("should error at creation time if the repo already does exist", func() { @@ -208,17 +211,13 @@ var _ = Describe("GitHub Provider", func() { Expect(actionTaken).To(BeTrue()) validateRepo(newRepo, repoRef) - /* TODO: Create an equality method for "settable" fields of the repository object - Comparing two API objects to each other using reflect.DeepEqual or JSON doesn't work - as current status is conflated in the same object and will result in race conditions. - // Reconcile by setting an "internal" field and updating it r := newRepo.APIObject().(*github.Repository) r.DeleteBranchOnMerge = gitprovider.BoolVar(true) actionTaken, err = newRepo.Reconcile(ctx) // Expect the update to succeed, and modify the state Expect(err).ToNot(HaveOccurred()) - Expect(actionTaken).To(BeTrue())*/ + Expect(actionTaken).To(BeTrue()) }) AfterSuite(func() { diff --git a/github/resource_deploykey.go b/github/resource_deploykey.go index 812ba254..1ed13c93 100644 --- a/github/resource_deploykey.go +++ b/github/resource_deploykey.go @@ -123,8 +123,12 @@ func (dk *deployKey) Reconcile(ctx context.Context) (bool, error) { return false, fmt.Errorf("expected to be able to cast actual to *deployKey: %w", gitprovider.ErrUnexpectedEvent) } + // Use wrappers here to extract the "spec" part of the object for comparison + desiredSpec := newGithubKeySpec(&dk.k) + actualSpec := newGithubKeySpec(&actualKey.k) + // If the desired matches the actual state, do nothing - if reflect.DeepEqual(dk.k, actualKey.k) { + if desiredSpec.Equals(actualSpec) { return false, nil } // If desired and actual state mis-match, update @@ -190,3 +194,25 @@ func deployKeyInfoToAPIObj(info *gitprovider.DeployKeyInfo, apiObj *github.Key) apiObj.ReadOnly = info.ReadOnly } } + +// This function copies over the fields that are part of create request of a deploy +// i.e. the desired spec of the deploy key. This allows us to separate "spec" from "status" fields. +func newGithubKeySpec(key *github.Key) *githubKeySpec { + return &githubKeySpec{ + &github.Key{ + // Create-specific parameters + // See: https://docs.github.com/en/rest/reference/repos#create-a-deploy-key + Title: key.Title, + Key: key.Key, + ReadOnly: key.ReadOnly, + }, + } +} + +type githubKeySpec struct { + *github.Key +} + +func (s *githubKeySpec) Equals(other *githubKeySpec) bool { + return reflect.DeepEqual(s, other) +} diff --git a/github/resource_repository.go b/github/resource_repository.go index ab80c3a7..610c35e6 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -124,12 +124,12 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) { return false, err } - // If desired state already is actual, just return - // TODO: Create an equality method for "settable" fields of the repository object - // Comparing two API objects to each other using reflect.DeepEqual or JSON doesn't work - // as current status is conflated in the same object and will result in race conditions. - // For now, as a workaround, we'll just compare the RepositoryInfo with each other. - if reflect.DeepEqual(repositoryFromAPI(apiObj), r.Get()) { + // Use wrappers here to extract the "spec" part of the object for comparison + desiredSpec := newGithubRepositorySpec(&r.r) + actualSpec := newGithubRepositorySpec(apiObj) + + // If desired state already is the actual state, do nothing + if desiredSpec.Equals(actualSpec) { return false, nil } // Otherwise, make the desired state the actual state @@ -237,3 +237,48 @@ func applyRepoCreateOptions(apiObj *github.Repository, opts gitprovider.Reposito apiObj.LicenseTemplate = gitprovider.StringVar(string(*opts.LicenseTemplate)) } } + +// This function copies over the fields that are part of create/update requests of a repository +// i.e. the desired spec of the repository. This allows us to separate "spec" from "status" fields. +// See also: https://github.com/google/go-github/blob/master/github/repos.go#L340-L358 +func newGithubRepositorySpec(repo *github.Repository) *githubRepositorySpec { + return &githubRepositorySpec{ + &github.Repository{ + // Generic + Name: repo.Name, + Description: repo.Description, + Homepage: repo.Homepage, + Private: repo.Private, + Visibility: repo.Visibility, + HasIssues: repo.HasIssues, + HasProjects: repo.HasProjects, + HasWiki: repo.HasWiki, + IsTemplate: repo.IsTemplate, + + // Update-specific parameters + // See: https://docs.github.com/en/rest/reference/repos#update-a-repository + DefaultBranch: repo.DefaultBranch, + + // Create-specific parameters + // See: https://docs.github.com/en/rest/reference/repos#create-an-organization-repository + TeamID: repo.TeamID, + AutoInit: repo.AutoInit, + GitignoreTemplate: repo.GitignoreTemplate, + LicenseTemplate: repo.LicenseTemplate, + + // Generic + AllowSquashMerge: repo.AllowSquashMerge, + AllowMergeCommit: repo.AllowMergeCommit, + AllowRebaseMerge: repo.AllowRebaseMerge, + DeleteBranchOnMerge: repo.DeleteBranchOnMerge, + }, + } +} + +type githubRepositorySpec struct { + *github.Repository +} + +func (s *githubRepositorySpec) Equals(other *githubRepositorySpec) bool { + return reflect.DeepEqual(s, other) +} From 8f700e3d667599a8dfcaf2ab37483f1d196d2cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 16:29:19 +0300 Subject: [PATCH 03/19] Extract generic server data validation out to a helper func --- github/resource_deploykey.go | 38 ++++++++++++++++------------------- github/resource_repository.go | 27 ++++++++++++------------- github/util.go | 14 +++++++++++++ github/util_test.go | 30 +++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 35 deletions(-) diff --git a/github/resource_deploykey.go b/github/resource_deploykey.go index 1ed13c93..22a6d8c4 100644 --- a/github/resource_deploykey.go +++ b/github/resource_deploykey.go @@ -148,27 +148,23 @@ func (dk *deployKey) createIntoSelf(ctx context.Context) error { } func validateDeployKeyAPI(apiObj *github.Key) error { - validator := validation.New("GitHub.Key") - // Make sure ID, title, key and readonly fields are populated as per - // https://docs.github.com/en/rest/reference/repos#get-a-deploy-key - // and similar docs - if apiObj.ID == nil { - validator.Required("ID") - } - if apiObj.Title == nil { - validator.Required("Title") - } - if apiObj.Key == nil { - validator.Required("Key") - } - if apiObj.ReadOnly == nil { - validator.Required("ReadOnly") - } - // If there was a validation error, also mark it specifically as invalid server data - if err := validator.Error(); err != nil { - return validation.NewMultiError(err, gitprovider.ErrInvalidServerData) - } - return nil + return validateAPIObject("GitHub.Key", func(validator validation.Validator) { + // Make sure ID, title, key and readonly fields are populated as per + // https://docs.github.com/en/rest/reference/repos#get-a-deploy-key + // and similar docs + if apiObj.ID == nil { + validator.Required("ID") + } + if apiObj.Title == nil { + validator.Required("Title") + } + if apiObj.Key == nil { + validator.Required("Key") + } + if apiObj.ReadOnly == nil { + validator.Required("ReadOnly") + } + }) } func deployKeyFromAPI(apiObj *github.Key) gitprovider.DeployKeyInfo { diff --git a/github/resource_repository.go b/github/resource_repository.go index 610c35e6..04082b80 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -171,21 +171,20 @@ func (r *orgRepository) TeamAccess() gitprovider.TeamAccessClient { return r.teamAccess } +// validateRepositoryAPI validates the apiObj received from the server, to make sure that it is +// valid for our use func validateRepositoryAPI(apiObj *github.Repository) error { - validator := validation.New("GitHub.Repository") - // Make sure name isn't nil - if apiObj.Name == nil { - validator.Required("Name") - } - if apiObj.Visibility != nil { - v := gitprovider.RepositoryVisibility(*apiObj.Visibility) - validator.Append(gitprovider.ValidateRepositoryVisibility(v), v, "Visibility") - } - // If there was a validation error, also mark it specifically as invalid server data - if err := validator.Error(); err != nil { - return validation.NewMultiError(err, gitprovider.ErrInvalidServerData) - } - return nil + return validateAPIObject("GitHub.Repository", func(validator validation.Validator) { + // Make sure name is set + if apiObj.Name == nil { + validator.Required("Name") + } + // Make sure visibility is valid if set + if apiObj.Visibility != nil { + v := gitprovider.RepositoryVisibility(*apiObj.Visibility) + validator.Append(gitprovider.ValidateRepositoryVisibility(v), v, "Visibility") + } + }) } func repositoryFromAPI(apiObj *github.Repository) gitprovider.RepositoryInfo { diff --git a/github/util.go b/github/util.go index b822d780..53d2b250 100644 --- a/github/util.go +++ b/github/util.go @@ -160,3 +160,17 @@ func allPages(opts *github.ListOptions, fn func() (*github.Response, error)) err opts.Page = resp.NextPage } } + +// validateAPIObject creates a Validatior with the specified name, gives it to fn, and +// depending on if any error was registered with it; either returns nil, or a MultiError +// with both the validation error and ErrInvalidServerData, to mark that the server data +// was invalid. +func validateAPIObject(name string, fn func(validation.Validator)) error { + v := validation.New(name) + fn(v) + // If there was a validation error, also mark it specifically as invalid server data + if err := v.Error(); err != nil { + return validation.NewMultiError(err, gitprovider.ErrInvalidServerData) + } + return nil +} diff --git a/github/util_test.go b/github/util_test.go index 029d9084..b957abb6 100644 --- a/github/util_test.go +++ b/github/util_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/fluxcd/go-git-providers/gitprovider" + "github.com/fluxcd/go-git-providers/validation" ) func Test_getPermissionFromMap(t *testing.T) { @@ -95,3 +96,32 @@ func Test_getPermissionFromMap(t *testing.T) { }) } } + +func Test_validateAPIObject(t *testing.T) { + tests := []struct { + name string + structName string + fn func(validation.Validator) + expectedErrs []error + }{ + { + name: "no error => nil", + structName: "Foo", + fn: func(validation.Validator) {}, + }, + { + name: "one error => MultiError & InvalidServerData", + structName: "Foo", + fn: func(v validation.Validator) { + v.Required("FieldBar") + }, + expectedErrs: []error{gitprovider.ErrInvalidServerData, &validation.MultiError{}, validation.ErrFieldRequired}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateAPIObject(tt.structName, tt.fn) + validation.TestExpectErrors(t, "validateAPIObject", err, tt.expectedErrs...) + }) + } +} From 8e56293b31680b4e8f5399799edbd4217c653fc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 17:15:27 +0300 Subject: [PATCH 04/19] Fix error checking with MultiError + structs --- validation/multierror.go | 3 ++- validation/multierror_test.go | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/validation/multierror.go b/validation/multierror.go index 7d5ca461..97ecd28e 100644 --- a/validation/multierror.go +++ b/validation/multierror.go @@ -124,7 +124,8 @@ func TestExpectErrors(t testing.TB, funcName string, err error, expectedErrs ... expectedType := reflect.TypeOf(expectedErr) expectedTypeName := expectedType.String() _, nameDisallowed := disallowedCompareAsErrorNames[expectedTypeName] - if expectedType == reflect.TypeOf(err) && !nameDisallowed { + _, isMultiError := err.(*MultiError) + if (expectedType == reflect.TypeOf(err) || isMultiError) && !nameDisallowed { target := expectedErr.(interface{}) if errors.As(err, &target) { continue diff --git a/validation/multierror_test.go b/validation/multierror_test.go index b33efca2..02dfeb35 100644 --- a/validation/multierror_test.go +++ b/validation/multierror_test.go @@ -212,6 +212,11 @@ func Test_ExpectErrors(t *testing.T) { err: &url.Error{Op: "foo", URL: "bar", Err: fmt.Errorf("baz")}, expectedErrs: []error{&url.Error{}}, }, + { + name: "multierror struct propagation", + err: &MultiError{Errors: []error{&url.Error{}}}, + expectedErrs: []error{&MultiError{}, &url.Error{}}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 609d6f893ea9d0aabfe4595af031ce2909226704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 17:15:44 +0300 Subject: [PATCH 05/19] Unit-test allPages, and make it wrap HTTP errors --- github/client_organization_teams.go | 4 +- github/client_organizations.go | 2 +- github/client_repositories_org.go | 2 +- github/client_repositories_user.go | 2 +- github/client_repository_deploykey.go | 2 +- github/client_repository_teamaccess.go | 2 +- github/util.go | 3 +- github/util_test.go | 82 ++++++++++++++++++++++++++ 8 files changed, 91 insertions(+), 8 deletions(-) diff --git a/github/client_organization_teams.go b/github/client_organization_teams.go index d3617c28..257a9afb 100644 --- a/github/client_organization_teams.go +++ b/github/client_organization_teams.go @@ -50,7 +50,7 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } logins := make([]string, 0, len(apiObjs)) @@ -83,7 +83,7 @@ func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) { return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } // Use .Get() to get detailed information about each member diff --git a/github/client_organizations.go b/github/client_organizations.go index db75e6c7..3a1d5b1a 100644 --- a/github/client_organizations.go +++ b/github/client_organizations.go @@ -64,7 +64,7 @@ func (c *OrganizationsClient) List(ctx context.Context) ([]gitprovider.Organizat return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } orgs := make([]gitprovider.Organization, 0, len(apiObjs)) diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go index 91a0b4b5..c0229ce1 100644 --- a/github/client_repositories_org.go +++ b/github/client_repositories_org.go @@ -68,7 +68,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } // Traverse the list, and return a list of OrgRepository objects diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go index 4a3df435..218478f4 100644 --- a/github/client_repositories_user.go +++ b/github/client_repositories_user.go @@ -67,7 +67,7 @@ func (c *UserRepositoriesClient) List(ctx context.Context, ref gitprovider.UserR return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } // Traverse the list, and return a list of UserRepository objects diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index 5c486b03..b1bbb424 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -68,7 +68,7 @@ func (c *DeployKeyClient) List(ctx context.Context) ([]gitprovider.DeployKey, er return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } // Map the api object to our DeployKey type diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go index 8fae04cc..008a28b3 100644 --- a/github/client_repository_teamaccess.go +++ b/github/client_repository_teamaccess.go @@ -65,7 +65,7 @@ func (c *TeamAccessClient) List(ctx context.Context) ([]gitprovider.TeamAccess, return resp, listErr }) if err != nil { - return nil, handleHTTPError(err) + return nil, err } teamAccess := make([]gitprovider.TeamAccess, 0, len(apiObjs)) diff --git a/github/util.go b/github/util.go index 53d2b250..0fa72d33 100644 --- a/github/util.go +++ b/github/util.go @@ -148,11 +148,12 @@ func handleHTTPError(err error) error { // allPages runs fn for each page, expecting a HTTP request to be made and returned during that call. // allPages expects that the data is saved in fn to an outer variable. // allPages calls fn as many times as needed to get all pages, and modifies opts for each call. +// There is no need to wrap the resulting error in handleHTTPError(err), as that's already done. func allPages(opts *github.ListOptions, fn func() (*github.Response, error)) error { for { resp, err := fn() if err != nil { - return err + return handleHTTPError(err) } if resp.NextPage == 0 { return nil diff --git a/github/util_test.go b/github/util_test.go index b957abb6..3b07fd6e 100644 --- a/github/util_test.go +++ b/github/util_test.go @@ -17,11 +17,14 @@ limitations under the License. package github import ( + "net/http" + "net/url" "reflect" "testing" "github.com/fluxcd/go-git-providers/gitprovider" "github.com/fluxcd/go-git-providers/validation" + "github.com/google/go-github/v32/github" ) func Test_getPermissionFromMap(t *testing.T) { @@ -125,3 +128,82 @@ func Test_validateAPIObject(t *testing.T) { }) } } + +func newGHError() *github.ErrorResponse { + return &github.ErrorResponse{ + Response: &http.Response{ + Request: &http.Request{ + Method: "GET", + URL: &url.URL{}, + }, + StatusCode: 404, + }, + } +} + +func Test_allPages(t *testing.T) { + tests := []struct { + name string + opts *github.ListOptions + fn func(int) (*github.Response, error) + expectedErrs []error + expectedCalls int + }{ + { + name: "one page only, no error", + opts: &github.ListOptions{}, + fn: func(_ int) (*github.Response, error) { + return &github.Response{NextPage: 0}, nil + }, + expectedCalls: 1, + }, + { + name: "two pages, no error", + opts: &github.ListOptions{}, + fn: func(i int) (*github.Response, error) { + switch i { + case 1: + return &github.Response{NextPage: 2}, nil + } + return &github.Response{NextPage: 0}, nil + }, + expectedCalls: 2, + }, + { + name: "four pages, error at second", + opts: &github.ListOptions{}, + fn: func(i int) (*github.Response, error) { + switch i { + case 1: + return &github.Response{NextPage: 2}, nil + case 2: + return nil, newGHError() + case 3: + return &github.Response{NextPage: 4}, nil + } + return &github.Response{NextPage: 0}, nil + }, + expectedCalls: 2, + expectedErrs: []error{&validation.MultiError{}, gitprovider.ErrNotFound, newGHError()}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + i := 0 + // the page index are 1-based, and omitting page is the same as page=1 + // set page=1 here just to be able to test more easily + tt.opts.Page = 1 + err := allPages(tt.opts, func() (*github.Response, error) { + i++ + if tt.opts.Page != i { + t.Fatalf("page number is unexpected: got = %d want = %d", tt.opts.Page, i) + } + return tt.fn(i) + }) + validation.TestExpectErrors(t, "allPages", err, tt.expectedErrs...) + if i != tt.expectedCalls { + t.Errorf("allPages() expectedCalls = %v, want %v", i, tt.expectedCalls) + } + }) + } +} From a7ab67ec080c17a2b7fc8fb47e90fdc7cfb16fbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 17:35:01 +0300 Subject: [PATCH 06/19] nil-check member login --- github/client_organization_teams.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/github/client_organization_teams.go b/github/client_organization_teams.go index 257a9afb..51ebde7a 100644 --- a/github/client_organization_teams.go +++ b/github/client_organization_teams.go @@ -55,8 +55,11 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea logins := make([]string, 0, len(apiObjs)) for _, apiObj := range apiObjs { - // TODO: Maybe check for non-empty logins? - logins = append(logins, apiObj.GetLogin()) + // Make sure login isn't nil + if apiObj.Login == nil { + return nil, fmt.Errorf("didn't expect login to be nil for user: %+v: %w", apiObj, gitprovider.ErrInvalidServerData) + } + logins = append(logins, *apiObj.Login) } return &team{ From b99ef75cdf457e332bb0cd31cff9a4d0c23cfcba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 17:35:56 +0300 Subject: [PATCH 07/19] remove unnecessary cast of deploykey --- github/client_repository_deploykey.go | 24 ++++++++++++++++++++---- github/resource_deploykey.go | 10 ++-------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index b1bbb424..2efdb1d0 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -39,17 +39,20 @@ type DeployKeyClient struct { // // ErrNotFound is returned if the resource does not exist. func (c *DeployKeyClient) Get(ctx context.Context, name string) (gitprovider.DeployKey, error) { + return c.get(ctx, name) +} - deployKeys, err := c.List(ctx) +func (c *DeployKeyClient) get(ctx context.Context, name string) (*deployKey, error) { + deployKeys, err := c.list(ctx) if err != nil { return nil, err } + // Loop through deploy keys once we find one with the right name for _, dk := range deployKeys { - if dk.Get().Name == name { + if *dk.k.Title == name { return dk, nil } } - return nil, gitprovider.ErrNotFound } @@ -58,6 +61,19 @@ func (c *DeployKeyClient) Get(ctx context.Context, name string) (gitprovider.Dep // List returns all available repository deploy keys for the given type, // using multiple paginated requests if needed. func (c *DeployKeyClient) List(ctx context.Context) ([]gitprovider.DeployKey, error) { + dks, err := c.list(ctx) + if err != nil { + return nil, err + } + // Cast to []gitprovider.DeployKey + keys := make([]gitprovider.DeployKey, 0, len(dks)) + for _, dk := range dks { + keys = append(keys, dk) + } + return keys, nil +} + +func (c *DeployKeyClient) list(ctx context.Context) ([]*deployKey, error) { // List all keys, using pagination. apiObjs := []*github.Key{} opts := &github.ListOptions{} @@ -72,7 +88,7 @@ func (c *DeployKeyClient) List(ctx context.Context) ([]gitprovider.DeployKey, er } // Map the api object to our DeployKey type - keys := make([]gitprovider.DeployKey, 0, len(apiObjs)) + keys := make([]*deployKey, 0, len(apiObjs)) for _, apiObj := range apiObjs { k, err := newDeployKey(c, apiObj) if err != nil { diff --git a/github/resource_deploykey.go b/github/resource_deploykey.go index 22a6d8c4..cafcfdfc 100644 --- a/github/resource_deploykey.go +++ b/github/resource_deploykey.go @@ -106,7 +106,7 @@ func (dk *deployKey) Delete(ctx context.Context) error { // // The internal API object will be overridden with the received server data if actionTaken == true. func (dk *deployKey) Reconcile(ctx context.Context) (bool, error) { - actual, err := dk.c.Get(ctx, *dk.k.Key) + actual, err := dk.c.get(ctx, *dk.k.Key) if err != nil { // Create if not found if errors.Is(err, gitprovider.ErrNotFound) { @@ -117,15 +117,9 @@ func (dk *deployKey) Reconcile(ctx context.Context) (bool, error) { return false, err } - // This should never (tm) fail, but just to make sure, return an error and don't panic - actualKey, ok := actual.(*deployKey) - if !ok { - return false, fmt.Errorf("expected to be able to cast actual to *deployKey: %w", gitprovider.ErrUnexpectedEvent) - } - // Use wrappers here to extract the "spec" part of the object for comparison desiredSpec := newGithubKeySpec(&dk.k) - actualSpec := newGithubKeySpec(&actualKey.k) + actualSpec := newGithubKeySpec(&actual.k) // If the desired matches the actual state, do nothing if desiredSpec.Equals(actualSpec) { From 26f371c54dc3bfe1bb2ba618d9dad7027e4dae27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 17:52:07 +0300 Subject: [PATCH 08/19] fix typos --- github/auth.go | 6 +++--- github/integration_test.go | 2 +- github/resource_repository.go | 4 ++-- gitprovider/gitprovider.go | 2 +- gitprovider/resources.go | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/github/auth.go b/github/auth.go index f4a47dbd..63906c77 100644 --- a/github/auth.go +++ b/github/auth.go @@ -54,7 +54,7 @@ type clientOptions struct { // GitHub Enterprise. If unset, defaultDomain will be used. Domain *string - // ClientFactory is a way to aquire a *http.Client, possibly with auth credentials + // ClientFactory is a way to acquire a *http.Client, possibly with auth credentials ClientFactory ClientFactory // EnableDestructiveAPICalls is a flag to tell whether destructive API calls like @@ -102,7 +102,7 @@ func WithPersonalAccessToken(patToken string) ClientOption { } } -// WithClientFactory initializes a Client with a given ClientFactory, used for aquiring the *http.Client later. +// WithClientFactory initializes a Client with a given ClientFactory, used for acquiring the *http.Client later. // clientFactory must not be nil. // WithClientFactory is mutually exclusive with WithOAuth2Token and WithPersonalAccessToken. func WithClientFactory(clientFactory ClientFactory) ClientOption { @@ -150,7 +150,7 @@ func WithDestructiveAPICalls(destructiveActions bool) ClientOption { } } -// ClientFactory is a way to aquire a *http.Client, possibly with auth credentials +// ClientFactory is a way to acquire a *http.Client, possibly with auth credentials type ClientFactory interface { // Client returns a *http.Client, possibly with auth credentials Client(ctx context.Context) *http.Client diff --git a/github/integration_test.go b/github/integration_test.go index 6377c54c..26844e06 100644 --- a/github/integration_test.go +++ b/github/integration_test.go @@ -70,7 +70,7 @@ var _ = Describe("GitHub Provider", func() { if token := string(b); err == nil && len(token) != 0 { githubToken = token } else { - Fail("couldn't aquire GITHUB_TOKEN env variable") + Fail("couldn't acquire GITHUB_TOKEN env variable") } } diff --git a/github/resource_repository.go b/github/resource_repository.go index 04082b80..a61d0ca3 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -136,11 +136,11 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) { return true, r.Update(ctx) } -// Delete deletes the current resource irreversebly. +// Delete deletes the current resource irreversible. // // ErrNotFound is returned if the resource doesn't exist anymore. func (r *userRepository) Delete(ctx context.Context) error { - // Don't allow deleting repositories if the user didn't explicitely allow dangerous API calls. + // Don't allow deleting repositories if the user didn't explicitly allow dangerous API calls. if !r.destructiveActions { return fmt.Errorf("cannot delete repository: %w", ErrDestructiveCallDisallowed) } diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 070e966f..454090d9 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -51,7 +51,7 @@ type GenericUpdatable interface { // GenericDeletable is an interface which all objects that can be deleted // using the Client implement. type GenericDeletable interface { - // Delete deletes the current resource irreversebly. + // Delete deletes the current resource irreversible. // // ErrNotFound is returned if the resource doesn't exist anymore. Delete(ctx context.Context) error diff --git a/gitprovider/resources.go b/gitprovider/resources.go index 911f5520..34a66695 100644 --- a/gitprovider/resources.go +++ b/gitprovider/resources.go @@ -69,7 +69,7 @@ type UserRepository interface { DeployKeys() DeployKeyClient } -// OrgRepository describes a respository owned by an organization. +// OrgRepository describes a repository owned by an organization. type OrgRepository interface { // OrgRepository is a superset of UserRepository. UserRepository From 593d5956a5d6834201e8f2c07f95ae96bd97c197 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 18:06:47 +0300 Subject: [PATCH 09/19] Fix golint errors --- bitbucket/doc.go | 1 + github/client.go | 1 + github/client_organizations.go | 1 + github/client_repositories_org.go | 12 ++++++------ github/client_repositories_user.go | 4 ++-- github/client_repository_deploykey.go | 6 +++--- github/client_repository_teamaccess.go | 12 +++++++++--- github/resource_deploykey.go | 2 +- github/resource_repository.go | 4 ++-- gitlab/doc.go | 1 + gitprovider/client.go | 2 +- gitprovider/types_organization.go | 2 +- gitprovider/types_repository.go | 2 ++ 13 files changed, 31 insertions(+), 19 deletions(-) diff --git a/bitbucket/doc.go b/bitbucket/doc.go index 4b11a0f9..203cf7c8 100644 --- a/bitbucket/doc.go +++ b/bitbucket/doc.go @@ -17,5 +17,6 @@ limitations under the License. package bitbucket import ( + // Dummy import until we have the implementation ready _ "github.com/ktrysmt/go-bitbucket" ) diff --git a/github/client.go b/github/client.go index cc74a462..01d73cad 100644 --- a/github/client.go +++ b/github/client.go @@ -50,6 +50,7 @@ type clientContext struct { // Client implements the gitprovider.Client interface var _ gitprovider.Client = &Client{} +// Client is an interface that allows talking to a Git provider. type Client struct { *clientContext diff --git a/github/client_organizations.go b/github/client_organizations.go index 3a1d5b1a..de2abfba 100644 --- a/github/client_organizations.go +++ b/github/client_organizations.go @@ -28,6 +28,7 @@ import ( // OrganizationsClient implements the gitprovider.OrganizationsClient interface var _ gitprovider.OrganizationsClient = &OrganizationsClient{} +// OrganizationsClient operates on organizations the user has access to. type OrganizationsClient struct { *clientContext } diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go index c0229ce1..c1596283 100644 --- a/github/client_repositories_org.go +++ b/github/client_repositories_org.go @@ -42,7 +42,7 @@ func (c *OrgRepositoriesClient) Get(ctx context.Context, ref gitprovider.OrgRepo if err := validateOrgRepositoryRef(ref, c.domain); err != nil { return nil, err } - apiObj, err := getRepository(c.c, ctx, ref) + apiObj, err := getRepository(ctx, c.c, ref) if err != nil { return nil, err } @@ -96,7 +96,7 @@ func (c *OrgRepositoriesClient) Create(ctx context.Context, ref gitprovider.OrgR return nil, err } - apiObj, err := createRepository(c.c, ctx, ref, ref.Organization, req, opts...) + apiObj, err := createRepository(ctx, c.c, ref, ref.Organization, req, opts...) if err != nil { return nil, err } @@ -130,13 +130,13 @@ func (c *OrgRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.O return actual, actionTaken, err } -func getRepository(c *github.Client, ctx context.Context, ref gitprovider.RepositoryRef) (*github.Repository, error) { +func getRepository(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef) (*github.Repository, error) { // GET /repos/{owner}/{repo} apiObj, _, err := c.Repositories.Get(ctx, ref.GetIdentity(), ref.GetRepository()) return validateRepositoryAPIResp(apiObj, err) } -func createRepository(c *github.Client, ctx context.Context, ref gitprovider.RepositoryRef, orgName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*github.Repository, error) { +func createRepository(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, orgName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*github.Repository, error) { // Make sure the request is valid if err := req.ValidateInfo(); err != nil { return nil, err @@ -153,10 +153,10 @@ func createRepository(c *github.Client, ctx context.Context, ref gitprovider.Rep data := repositoryToAPI(&req, ref) applyRepoCreateOptions(&data, o) - return createRepositoryData(c, ctx, orgName, &data) + return createRepositoryData(ctx, c, orgName, &data) } -func createRepositoryData(c *github.Client, ctx context.Context, orgName string, data *github.Repository) (*github.Repository, error) { +func createRepositoryData(ctx context.Context, c *github.Client, orgName string, data *github.Repository) (*github.Repository, error) { // POST /user/repos or // POST /orgs/{org}/repos // depending on orgName diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go index 218478f4..c6727dd3 100644 --- a/github/client_repositories_user.go +++ b/github/client_repositories_user.go @@ -41,7 +41,7 @@ func (c *UserRepositoriesClient) Get(ctx context.Context, ref gitprovider.UserRe if err := validateUserRepositoryRef(ref, c.domain); err != nil { return nil, err } - apiObj, err := getRepository(c.c, ctx, ref) + apiObj, err := getRepository(ctx, c.c, ref) if err != nil { return nil, err } @@ -95,7 +95,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, ref gitprovider.Use return nil, err } - apiObj, err := createRepository(c.c, ctx, ref, "", req, opts...) + apiObj, err := createRepository(ctx, c.c, ref, "", req, opts...) if err != nil { return nil, err } diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index 2efdb1d0..606d0a50 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -148,17 +148,17 @@ func (c *DeployKeyClient) Reconcile(ctx context.Context, req gitprovider.DeployK return actual, true, actual.Update(ctx) } -func createDeployKey(c *github.Client, ctx context.Context, ref gitprovider.RepositoryRef, req gitprovider.DeployKeyInfo) (*github.Key, error) { +func createDeployKey(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, req gitprovider.DeployKeyInfo) (*github.Key, error) { // Validate the create request and default if err := req.ValidateInfo(); err != nil { return nil, err } req.Default() - return createDeployKeyData(c, ctx, ref, deployKeyToAPI(&req)) + return createDeployKeyData(ctx, c, ref, deployKeyToAPI(&req)) } -func createDeployKeyData(c *github.Client, ctx context.Context, ref gitprovider.RepositoryRef, data *github.Key) (*github.Key, error) { +func createDeployKeyData(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, data *github.Key) (*github.Key, error) { // POST /repos/{owner}/{repo}/keys apiObj, _, err := c.Repositories.CreateKey(ctx, ref.GetIdentity(), ref.GetRepository(), data) return apiObj, handleHTTPError(err) diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go index 008a28b3..8dd324bb 100644 --- a/github/client_repository_teamaccess.go +++ b/github/client_repository_teamaccess.go @@ -36,9 +36,15 @@ type TeamAccessClient struct { ref gitprovider.RepositoryRef } -func (c *TeamAccessClient) Get(ctx context.Context, teamName string) (gitprovider.TeamAccess, error) { +// Get a team within the specific organization. +// +// name may include slashes, but must not be an empty string. +// Teams are sub-groups in GitLab. +// +// ErrNotFound is returned if the resource does not exist. +func (c *TeamAccessClient) Get(ctx context.Context, name string) (gitprovider.TeamAccess, error) { // GET /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo} - apiObj, _, err := c.c.Teams.IsTeamRepoBySlug(ctx, c.ref.GetIdentity(), teamName, c.ref.GetIdentity(), c.ref.GetRepository()) + apiObj, _, err := c.c.Teams.IsTeamRepoBySlug(ctx, c.ref.GetIdentity(), name, c.ref.GetIdentity(), c.ref.GetRepository()) if err != nil { return nil, handleHTTPError(err) } @@ -48,7 +54,7 @@ func (c *TeamAccessClient) Get(ctx context.Context, teamName string) (gitprovide return nil, fmt.Errorf("didn't expect permissions to be nil for team access object: %+v: %w", apiObj, gitprovider.ErrInvalidServerData) } - return newTeamAccess(c, teamAccessFromAPI(apiObj, teamName)), nil + return newTeamAccess(c, teamAccessFromAPI(apiObj, name)), nil } // List lists the team access control list for this repository. diff --git a/github/resource_deploykey.go b/github/resource_deploykey.go index cafcfdfc..43338905 100644 --- a/github/resource_deploykey.go +++ b/github/resource_deploykey.go @@ -130,7 +130,7 @@ func (dk *deployKey) Reconcile(ctx context.Context) (bool, error) { } func (dk *deployKey) createIntoSelf(ctx context.Context) error { - apiObj, err := createDeployKeyData(dk.c.c, ctx, dk.c.ref, &dk.k) + apiObj, err := createDeployKeyData(ctx, dk.c.c, dk.c.ref, &dk.k) if err != nil { return err } diff --git a/github/resource_repository.go b/github/resource_repository.go index a61d0ca3..32ac1112 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -105,7 +105,7 @@ func (r *userRepository) Update(ctx context.Context) error { // // The internal API object will be overridden with the received server data if actionTaken == true. func (r *userRepository) Reconcile(ctx context.Context) (bool, error) { - apiObj, err := getRepository(r.c, ctx, r.ref) + apiObj, err := getRepository(ctx, r.c, r.ref) if err != nil { // Create if not found if errors.Is(err, gitprovider.ErrNotFound) { @@ -113,7 +113,7 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) { if orgRef, ok := r.ref.(gitprovider.OrgRepositoryRef); ok { orgName = orgRef.Organization } - repo, err := createRepositoryData(r.c, ctx, orgName, &r.r) + repo, err := createRepositoryData(ctx, r.c, orgName, &r.r) if err != nil { return true, err } diff --git a/gitlab/doc.go b/gitlab/doc.go index d7245e5f..8b0d7262 100644 --- a/gitlab/doc.go +++ b/gitlab/doc.go @@ -17,5 +17,6 @@ limitations under the License. package gitlab import ( + // Dummy import until we have the implementation ready _ "github.com/xanzy/go-gitlab" ) diff --git a/gitprovider/client.go b/gitprovider/client.go index 81589bf9..096f30b9 100644 --- a/gitprovider/client.go +++ b/gitprovider/client.go @@ -102,7 +102,7 @@ type OrgRepositoriesClient interface { Reconcile(ctx context.Context, r OrgRepositoryRef, req RepositoryInfo, opts ...RepositoryReconcileOption) (resp OrgRepository, actionTaken bool, err error) } -// OrgRepositoriesClient operates on repositories for users.. +// UserRepositoriesClient operates on repositories for users. type UserRepositoriesClient interface { // Get returns the repository at the given path. // diff --git a/gitprovider/types_organization.go b/gitprovider/types_organization.go index 1dcf1ed3..fa972b9c 100644 --- a/gitprovider/types_organization.go +++ b/gitprovider/types_organization.go @@ -25,7 +25,7 @@ type OrganizationInfo struct { Description *string `json:"description"` } -// Team is a representation for a team of users inside of an organization +// TeamInfo is a representation for a team of users inside of an organization. type TeamInfo struct { // Name describes the name of the team. The team name may contain slashes. Name string `json:"name"` diff --git a/gitprovider/types_repository.go b/gitprovider/types_repository.go index 3efb2fda..6161ee86 100644 --- a/gitprovider/types_repository.go +++ b/gitprovider/types_repository.go @@ -79,6 +79,7 @@ func (r *RepositoryInfo) ValidateInfo() error { // TeamAccess can be created and deleted var _ CreatableInfo = &TeamAccessInfo{} +// TeamAccessInfo contains high-level information about a team's access to a repository. type TeamAccessInfo struct { // Name describes the name of the team. The team name may contain slashes // +required @@ -114,6 +115,7 @@ func (ta *TeamAccessInfo) ValidateInfo() error { var _ CreatableInfo = &DeployKeyInfo{} +// DeployKeyInfo contains high-level information about a deploy key. type DeployKeyInfo struct { // Name is the human-friendly interpretation of what the key is for (and does) // +required From 644238f6640a3841c728eda9f42845901db4c1d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 18:44:31 +0300 Subject: [PATCH 10/19] Add many more linters to golangci config, don't run on every push not covered by a PR --- .github/workflows/golangci.yaml | 8 +++-- .golangci.yml | 58 +++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/golangci.yaml b/.github/workflows/golangci.yaml index 180707f3..d067e10d 100644 --- a/.github/workflows/golangci.yaml +++ b/.github/workflows/golangci.yaml @@ -1,6 +1,10 @@ # https://github.com/marketplace/actions/run-golangci-lint name: golangci-lint -on: [push, pull_request] +on: + pull_request: + push: + branches: + - master jobs: golangci: name: lint @@ -11,4 +15,4 @@ jobs: uses: golangci/golangci-lint-action@v1 with: # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. - version: v1.28 + version: v1.30 diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..c53a2a45 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,58 @@ +run: + # timeout for analysis, e.g. 30s, 5m, default is 1m + timeout: 2m + +linters: + enable: + # Default + - govet + - errcheck + - staticcheck + - unused + - gosimple + - structcheck + - varcheck + - ineffassign + - deadcode + - typecheck + # Extra, see list of https://golangci-lint.run/usage/linters/ + - bodyclose + - golint + # - rowserrcheck is not applicable here + - stylecheck + - gosec + # - interfacer is deprecated + - unconvert + - dupl + - goconst + - gocyclo + - gocognit + - asciicheck + - gofmt + - goimports + - maligned + # - depguard is not applicable here + - misspell + - lll + - unparam + - dogsled + - nakedret + - prealloc + - scopelint + - gocritic + - gochecknoinits + - gochecknoglobals + - godox + - funlen + - whitespace + - wsl + - goprintffuncname + - gomnd + - goerr113 + # - gomodguard is not applicable here + - godot + # - testpackage is not applicable here + # - nestif is covered by gocognit + # - exportloopref is covered by scopelint + - exhaustive + - nolintlint From 86165a881a3d07c0774c60cb5f0dc5feed55b97e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Wed, 12 Aug 2020 18:54:40 +0300 Subject: [PATCH 11/19] fix small error --- github/client_repository_deploykey.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index 606d0a50..d92837ee 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -104,7 +104,7 @@ func (c *DeployKeyClient) list(ctx context.Context) ([]*deployKey, error) { // // ErrAlreadyExists will be returned if the resource already exists. func (c *DeployKeyClient) Create(ctx context.Context, req gitprovider.DeployKeyInfo) (gitprovider.DeployKey, error) { - apiObj, err := createDeployKey(c.c, ctx, c.ref, req) + apiObj, err := createDeployKey(ctx, c.c, c.ref, req) if err != nil { return nil, err } From 908e15368825e5f161a8787b38ed83998dd38e5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 13:58:05 +0300 Subject: [PATCH 12/19] Fix lint errors (mostly godot). Maybe re-enable some of the now disabled linters later. --- .golangci.yml | 9 ++-- bitbucket/doc.go | 2 +- github/auth.go | 20 ++++---- github/client.go | 10 ++-- github/client_organization_teams.go | 8 ++-- github/client_organizations.go | 3 +- github/client_repositories_org.go | 6 +-- github/client_repositories_user.go | 10 ++-- github/client_repository_deploykey.go | 4 +- github/client_repository_teamaccess.go | 8 ++-- github/integration_test.go | 5 +- github/resource_repository.go | 2 +- github/resource_teamaccess.go | 9 +++- github/util.go | 4 +- gitlab/doc.go | 2 +- gitprovider/enums.go | 42 +++++++++-------- gitprovider/errors.go | 52 ++++++++++----------- gitprovider/gitprovider.go | 8 ++-- gitprovider/options.go | 26 +++++------ gitprovider/repositoryref.go | 64 +++++++++++++------------- gitprovider/types_organization.go | 6 +-- gitprovider/types_repository.go | 53 ++++++++++----------- gitprovider/util.go | 4 +- validation/multierror.go | 11 +++-- validation/validation.go | 12 ++--- 25 files changed, 200 insertions(+), 180 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index c53a2a45..4db6889e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,7 @@ run: # timeout for analysis, e.g. 30s, 5m, default is 1m - timeout: 2m + timeout: 1m + tests: false linters: enable: @@ -33,7 +34,7 @@ linters: - maligned # - depguard is not applicable here - misspell - - lll + # - lll allow long lines for now, maybe revisit this later - unparam - dogsled - nakedret @@ -42,10 +43,10 @@ linters: - gocritic - gochecknoinits - gochecknoglobals - - godox + # - godox don't error although there are TODOs in the code - funlen - whitespace - - wsl + # - wsl allow "non-ideomatic" whitespace formattings for now - goprintffuncname - gomnd - goerr113 diff --git a/bitbucket/doc.go b/bitbucket/doc.go index 203cf7c8..cb08392e 100644 --- a/bitbucket/doc.go +++ b/bitbucket/doc.go @@ -17,6 +17,6 @@ limitations under the License. package bitbucket import ( - // Dummy import until we have the implementation ready + // Dummy import until we have the implementation ready. _ "github.com/ktrysmt/go-bitbucket" ) diff --git a/github/auth.go b/github/auth.go index 63906c77..cdab6e6e 100644 --- a/github/auth.go +++ b/github/auth.go @@ -29,7 +29,7 @@ import ( ) const ( - // defaultDomain specifies the default domain used as the backend + // defaultDomain specifies the default domain used as the backend. defaultDomain = "github.com" // patUsername is the "username" for the basic auth authentication flow // when using a personal access token as the "password". This string could @@ -40,7 +40,7 @@ const ( var ( // ErrInvalidClientOptions is the error returned when calling NewClient() with - // invalid options (e.g. specifying mutually exclusive options) + // invalid options (e.g. specifying mutually exclusive options). ErrInvalidClientOptions = errors.New("invalid options given to NewClient()") // ErrDestructiveCallDisallowed happens when the client isn't set up with WithDestructiveAPICalls() // but a destructive action is called. @@ -48,7 +48,7 @@ var ( ) // clientOptions is the struct that tracks data about what options have been set -// It is private so that the user must use the With... functions +// It is private so that the user must use the With... functions. type clientOptions struct { // Domain specifies the backing domain, which can be arbitrary if the user uses // GitHub Enterprise. If unset, defaultDomain will be used. @@ -79,6 +79,7 @@ func WithOAuth2Token(oauth2Token string) ClientOption { if opts.ClientFactory != nil { return fmt.Errorf("authentication http.Client already configured: %w", ErrInvalidClientOptions) } + opts.ClientFactory = &oauth2Auth{oauth2Token} return nil } @@ -150,18 +151,18 @@ func WithDestructiveAPICalls(destructiveActions bool) ClientOption { } } -// ClientFactory is a way to acquire a *http.Client, possibly with auth credentials +// ClientFactory is a way to acquire a *http.Client, possibly with auth credentials. type ClientFactory interface { - // Client returns a *http.Client, possibly with auth credentials + // Client returns a *http.Client, possibly with auth credentials. Client(ctx context.Context) *http.Client } -// oauth2Auth is an implementation of ClientFactory +// oauth2Auth is an implementation of ClientFactory. type oauth2Auth struct { token string } -// Client returns a *http.Client, possibly with auth credentials +// Client returns a *http.Client, possibly with auth credentials. func (a *oauth2Auth) Client(ctx context.Context) *http.Client { ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: a.token}, @@ -169,12 +170,12 @@ func (a *oauth2Auth) Client(ctx context.Context) *http.Client { return oauth2.NewClient(ctx, ts) } -// patAuth is an implementation of ClientFactory +// patAuth is an implementation of ClientFactory. type patAuth struct { token string } -// Client returns a *http.Client, possibly with auth credentials +// Client returns a *http.Client, possibly with auth credentials. func (a *patAuth) Client(ctx context.Context) *http.Client { auth := github.BasicAuthTransport{ Username: patUsername, @@ -223,6 +224,7 @@ func NewClient(ctx context.Context, optFns ...ClientOption) (gitprovider.Client, // the default. var gh *github.Client var domain string + if opts.Domain == nil || *opts.Domain == defaultDomain { // No domain or the default github.com used domain = defaultDomain diff --git a/github/client.go b/github/client.go index 01d73cad..0ae5e70f 100644 --- a/github/client.go +++ b/github/client.go @@ -22,7 +22,7 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// ProviderID is the provider ID for GitHub +// ProviderID is the provider ID for GitHub. const ProviderID = gitprovider.ProviderID("github") func newClient(c *github.Client, domain string, destructiveActions bool) *Client { @@ -47,7 +47,7 @@ type clientContext struct { destructiveActions bool } -// Client implements the gitprovider.Client interface +// Client implements the gitprovider.Client interface. var _ gitprovider.Client = &Client{} // Client is an interface that allows talking to a Git provider. @@ -62,13 +62,13 @@ type Client struct { // SupportedDomain returns the domain endpoint for this client, e.g. "github.com", "enterprise.github.com" or // "my-custom-git-server.com:6443". This allows a higher-level user to know what Client to use for // what endpoints. -// This field is set at client creation time, and can't be changed +// This field is set at client creation time, and can't be changed. func (c *Client) SupportedDomain() string { return c.domain } -// ProviderID returns the provider ID "github" -// This field is set at client creation time, and can't be changed +// ProviderID returns the provider ID "github". +// This field is set at client creation time, and can't be changed. func (c *Client) ProviderID() gitprovider.ProviderID { return ProviderID } diff --git a/github/client_organization_teams.go b/github/client_organization_teams.go index 51ebde7a..b56edf96 100644 --- a/github/client_organization_teams.go +++ b/github/client_organization_teams.go @@ -25,10 +25,10 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// TeamsClient implements the gitprovider.TeamsClient interface +// TeamsClient implements the gitprovider.TeamsClient interface. var _ gitprovider.TeamsClient = &TeamsClient{} -// TeamsClient handles teams organization-wide +// TeamsClient handles teams organization-wide. type TeamsClient struct { *clientContext ref gitprovider.OrganizationRef @@ -59,6 +59,7 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea if apiObj.Login == nil { return nil, fmt.Errorf("didn't expect login to be nil for user: %+v: %w", apiObj, gitprovider.ErrInvalidServerData) } + logins = append(logins, *apiObj.Login) } @@ -72,7 +73,7 @@ func (c *TeamsClient) Get(ctx context.Context, teamName string) (gitprovider.Tea }, nil } -// List all teams (recursively, in terms of subgroups) within the specific organization +// List all teams (recursively, in terms of subgroups) within the specific organization. // // List returns all available organizations, using multiple paginated requests if needed. func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) { @@ -102,6 +103,7 @@ func (c *TeamsClient) List(ctx context.Context) ([]gitprovider.Team, error) { if err != nil { return nil, err } + teams = append(teams, team) } diff --git a/github/client_organizations.go b/github/client_organizations.go index de2abfba..85472714 100644 --- a/github/client_organizations.go +++ b/github/client_organizations.go @@ -25,7 +25,7 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// OrganizationsClient implements the gitprovider.OrganizationsClient interface +// OrganizationsClient implements the gitprovider.OrganizationsClient interface. var _ gitprovider.OrganizationsClient = &OrganizationsClient{} // OrganizationsClient operates on organizations the user has access to. @@ -74,6 +74,7 @@ func (c *OrganizationsClient) List(ctx context.Context) ([]gitprovider.Organizat if apiObj.Login == nil { return nil, fmt.Errorf("didn't expect login to be nil for org: %+v: %w", apiObj, gitprovider.ErrInvalidServerData) } + orgs = append(orgs, newOrganization(c.clientContext, apiObj, gitprovider.OrganizationRef{ Domain: c.domain, Organization: *apiObj.Login, diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go index c1596283..73988e15 100644 --- a/github/client_repositories_org.go +++ b/github/client_repositories_org.go @@ -26,10 +26,10 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// OrgRepositoriesClient implements the gitprovider.OrgRepositoriesClient interface +// OrgRepositoriesClient implements the gitprovider.OrgRepositoriesClient interface. var _ gitprovider.OrgRepositoriesClient = &OrgRepositoriesClient{} -// OrgRepositoriesClient operates on repositories the user has access to +// OrgRepositoriesClient operates on repositories the user has access to. type OrgRepositoriesClient struct { *clientContext } @@ -87,7 +87,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi return repos, nil } -// Create creates a repository for the given organization, with the data and options +// Create creates a repository for the given organization, with the data and options. // // ErrAlreadyExists will be returned if the resource already exists. func (c *OrgRepositoriesClient) Create(ctx context.Context, ref gitprovider.OrgRepositoryRef, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (gitprovider.OrgRepository, error) { diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go index c6727dd3..5cd0d9ff 100644 --- a/github/client_repositories_user.go +++ b/github/client_repositories_user.go @@ -25,10 +25,10 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// UserRepositoriesClient implements the gitprovider.UserRepositoriesClient interface +// UserRepositoriesClient implements the gitprovider.UserRepositoriesClient interface. var _ gitprovider.UserRepositoriesClient = &UserRepositoriesClient{} -// UserRepositoriesClient operates on repositories the user has access to +// UserRepositoriesClient operates on repositories the user has access to. type UserRepositoriesClient struct { *clientContext } @@ -89,7 +89,11 @@ func (c *UserRepositoriesClient) List(ctx context.Context, ref gitprovider.UserR // Create creates a repository for the given organization, with the data and options // // ErrAlreadyExists will be returned if the resource already exists. -func (c *UserRepositoriesClient) Create(ctx context.Context, ref gitprovider.UserRepositoryRef, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (gitprovider.UserRepository, error) { +func (c *UserRepositoriesClient) Create(ctx context.Context, + ref gitprovider.UserRepositoryRef, + req gitprovider.RepositoryInfo, + opts ...gitprovider.RepositoryCreateOption, +) (gitprovider.UserRepository, error) { // Make sure the RepositoryRef is valid if err := validateUserRepositoryRef(ref, c.domain); err != nil { return nil, err diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index d92837ee..11f0ad1f 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -26,10 +26,10 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// DeployKeyClient implements the gitprovider.DeployKeyClient interface +// DeployKeyClient implements the gitprovider.DeployKeyClient interface. var _ gitprovider.DeployKeyClient = &DeployKeyClient{} -// DeployKeyClient operates on the access deploy key list for a specific repository +// DeployKeyClient operates on the access deploy key list for a specific repository. type DeployKeyClient struct { *clientContext ref gitprovider.RepositoryRef diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go index 8dd324bb..bd832dcd 100644 --- a/github/client_repository_teamaccess.go +++ b/github/client_repository_teamaccess.go @@ -27,10 +27,10 @@ import ( "github.com/fluxcd/go-git-providers/gitprovider" ) -// TeamAccessClient implements the gitprovider.TeamAccessClient interface +// TeamAccessClient implements the gitprovider.TeamAccessClient interface. var _ gitprovider.TeamAccessClient = &TeamAccessClient{} -// TeamAccessClient operates on the teams list for a specific repository +// TeamAccessClient operates on the teams list for a specific repository. type TeamAccessClient struct { *clientContext ref gitprovider.RepositoryRef @@ -119,7 +119,9 @@ func (c *TeamAccessClient) Create(ctx context.Context, req gitprovider.TeamAcces // If req doesn't exist under the hood, it is created (actionTaken == true). // If req doesn't equal the actual state, the resource will be deleted and recreated (actionTaken == true). // If req is already the actual state, this is a no-op (actionTaken == false). -func (c *TeamAccessClient) Reconcile(ctx context.Context, req gitprovider.TeamAccessInfo) (gitprovider.TeamAccess, bool, error) { +func (c *TeamAccessClient) Reconcile(ctx context.Context, + req gitprovider.TeamAccessInfo, +) (gitprovider.TeamAccess, bool, error) { // First thing, validate the request if err := req.ValidateInfo(); err != nil { return nil, false, err diff --git a/github/integration_test.go b/github/integration_test.go index 26844e06..11d66fba 100644 --- a/github/integration_test.go +++ b/github/integration_test.go @@ -27,7 +27,6 @@ import ( "time" "github.com/google/go-github/v32/github" - githubapi "github.com/google/go-github/v32/github" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -115,7 +114,7 @@ var _ = Describe("GitHub Provider", func() { Expect(getOrg.Get().Name).ToNot(BeNil()) Expect(getOrg.Get().Description).ToNot(BeNil()) // Expect Name and Description to match their underlying data - internal := getOrg.APIObject().(*githubapi.Organization) + internal := getOrg.APIObject().(*github.Organization) Expect(getOrg.Get().Name).To(Equal(internal.Name)) Expect(getOrg.Get().Description).To(Equal(internal.Description)) }) @@ -267,7 +266,7 @@ func validateRepo(repo gitprovider.OrgRepository, expectedRepoRef gitprovider.Re Expect(*info.Visibility).To(Equal(gitprovider.RepositoryVisibilityPrivate)) Expect(*info.DefaultBranch).To(Equal(defaultBranch)) // Expect high-level fields to match their underlying data - internal := repo.APIObject().(*githubapi.Repository) + internal := repo.APIObject().(*github.Repository) Expect(repo.Repository().GetRepository()).To(Equal(*internal.Name)) Expect(repo.Repository().GetIdentity()).To(Equal(internal.Owner.GetLogin())) Expect(*info.Description).To(Equal(*internal.Description)) diff --git a/github/resource_repository.go b/github/resource_repository.go index 32ac1112..e4e83a36 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -172,7 +172,7 @@ func (r *orgRepository) TeamAccess() gitprovider.TeamAccessClient { } // validateRepositoryAPI validates the apiObj received from the server, to make sure that it is -// valid for our use +// valid for our use. func validateRepositoryAPI(apiObj *github.Repository) error { return validateAPIObject("GitHub.Repository", func(validator validation.Validator) { // Make sure name is set diff --git a/github/resource_teamaccess.go b/github/resource_teamaccess.go index c0ab1df3..e0ba3292 100644 --- a/github/resource_teamaccess.go +++ b/github/resource_teamaccess.go @@ -64,9 +64,13 @@ func (ta *teamAccess) Repository() gitprovider.RepositoryRef { // // ErrNotFound is returned if the resource does not exist. func (ta *teamAccess) Delete(ctx context.Context) error { - // DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo} - _, err := ta.c.c.Teams.RemoveTeamRepoBySlug(ctx, ta.c.ref.GetIdentity(), ta.ta.Name, ta.c.ref.GetIdentity(), ta.c.ref.GetRepository()) + _, err := ta.c.c.Teams.RemoveTeamRepoBySlug(ctx, + ta.c.ref.GetIdentity(), + ta.ta.Name, + ta.c.ref.GetIdentity(), + ta.c.ref.GetRepository(), + ) return handleHTTPError(err) } @@ -117,6 +121,7 @@ func teamAccessFromAPI(apiObj *github.Repository, teamName string) gitprovider.T } } +//nolint:gochecknoglobals,gomnd var permissionPriority = map[gitprovider.RepositoryPermission]int{ gitprovider.RepositoryPermissionPull: 1, gitprovider.RepositoryPermissionTriage: 2, diff --git a/github/util.go b/github/util.go index 0fa72d33..f940131c 100644 --- a/github/util.go +++ b/github/util.go @@ -75,7 +75,7 @@ func validateUserRef(ref gitprovider.UserRef, expectedDomain string) error { return validateIdentityFields(ref, expectedDomain) } -// validateIdentityFields makes sure the type of the IdentityRef is supported, and the domain is as expected +// validateIdentityFields makes sure the type of the IdentityRef is supported, and the domain is as expected. func validateIdentityFields(ref gitprovider.IdentityRef, expectedDomain string) error { // Make sure the expected domain is used if ref.GetDomain() != expectedDomain { @@ -93,7 +93,7 @@ func validateIdentityFields(ref gitprovider.IdentityRef, expectedDomain string) // handleHTTPError checks the type of err, and returns typed variants of it // However, it _always_ keeps the original error too, and just wraps it in a MultiError -// The consumer must use errors.Is and errors.As to check for equality and get data out of it +// The consumer must use errors.Is and errors.As to check for equality and get data out of it. func handleHTTPError(err error) error { // Short-circuit quickly if possible, allow always piping through this function if err == nil { diff --git a/gitlab/doc.go b/gitlab/doc.go index 8b0d7262..8d732db3 100644 --- a/gitlab/doc.go +++ b/gitlab/doc.go @@ -17,6 +17,6 @@ limitations under the License. package gitlab import ( - // Dummy import until we have the implementation ready + // Dummy import until we have the implementation ready. _ "github.com/xanzy/go-gitlab" ) diff --git a/gitprovider/enums.go b/gitprovider/enums.go index bb18edf7..639ac29c 100644 --- a/gitprovider/enums.go +++ b/gitprovider/enums.go @@ -18,9 +18,10 @@ package gitprovider import "github.com/fluxcd/go-git-providers/validation" -// TransportType is an enum specifying the transport type used when cloning a repository +// TransportType is an enum specifying the transport type used when cloning a repository. type TransportType string +//nolint:godot const ( // TransportTypeHTTPS specifies a clone URL of the form: // https:////[/].git @@ -33,21 +34,22 @@ const ( TransportTypeSSH = TransportType("ssh") ) -// RepositoryVisibility is an enum specifying the visibility of a repository +// RepositoryVisibility is an enum specifying the visibility of a repository. type RepositoryVisibility string const ( - // RepositoryVisibilityPublic specifies that the repository should be publicly accessible + // RepositoryVisibilityPublic specifies that the repository should be publicly accessible. RepositoryVisibilityPublic = RepositoryVisibility("public") // RepositoryVisibilityInternal specifies that the repository should accessible within the - // own organization + // own organization. RepositoryVisibilityInternal = RepositoryVisibility("internal") // RepositoryVisibilityPrivate specifies that the repository should only be accessible by - // specifically added team members + // specifically added team members. RepositoryVisibilityPrivate = RepositoryVisibility("private") ) -// knownRepositoryVisibilityValues is a map of known RepositoryVisibility values, used for validation +// knownRepositoryVisibilityValues is a map of known RepositoryVisibility values, used for validation. +//nolint:gochecknoglobals var knownRepositoryVisibilityValues = map[RepositoryVisibility]struct{}{ RepositoryVisibilityPublic: {}, RepositoryVisibilityInternal: {}, @@ -55,7 +57,7 @@ var knownRepositoryVisibilityValues = map[RepositoryVisibility]struct{}{ } // ValidateRepositoryVisibility validates a given RepositoryVisibility. -// Use as errs.Append(ValidateRepositoryVisibility(visibility), visibility, "FieldName") +// Use as errs.Append(ValidateRepositoryVisibility(visibility), visibility, "FieldName"). func ValidateRepositoryVisibility(r RepositoryVisibility) error { _, ok := knownRepositoryVisibilityValues[r] if !ok { @@ -64,38 +66,39 @@ func ValidateRepositoryVisibility(r RepositoryVisibility) error { return nil } -// RepositoryVisibilityVar returns a pointer to a RepositoryVisibility +// RepositoryVisibilityVar returns a pointer to a RepositoryVisibility. func RepositoryVisibilityVar(r RepositoryVisibility) *RepositoryVisibility { return &r } // RepositoryPermission is an enum specifying the access level for a certain team or person -// for a given repository +// for a given repository. type RepositoryPermission string const ( // RepositoryPermissionPull ("pull") - team members can pull, but not push to or administer this repository - // This is called "guest" in GitLab + // This is called "guest" in GitLab. RepositoryPermissionPull = RepositoryPermission("pull") // RepositoryPermissionTriage ("triage") - team members can proactively manage issues and pull requests without write access. - // This is called "reporter" in GitLab + // This is called "reporter" in GitLab. RepositoryPermissionTriage = RepositoryPermission("triage") // RepositoryPermissionPush ("push") - team members can pull and push, but not administer this repository - // This is called "developer" in GitLab + // This is called "developer" in GitLab. RepositoryPermissionPush = RepositoryPermission("push") // RepositoryPermissionMaintain ("maintain") - team members can manage the repository without access to sensitive or destructive actions. - // This is called "maintainer" in GitLab + // This is called "maintainer" in GitLab. RepositoryPermissionMaintain = RepositoryPermission("maintain") // RepositoryPermissionAdmin ("admin") - team members can pull, push and administer this repository - // This is called "admin" or "owner" in GitLab + // This is called "admin" or "owner" in GitLab. RepositoryPermissionAdmin = RepositoryPermission("admin") ) -// knownRepositoryVisibilityValues is a map of known RepositoryPermission values, used for validation +// knownRepositoryVisibilityValues is a map of known RepositoryPermission values, used for validation. +//nolint:gochecknoglobals var knownRepositoryPermissionValues = map[RepositoryPermission]struct{}{ RepositoryPermissionPull: {}, RepositoryPermissionTriage: {}, @@ -105,7 +108,7 @@ var knownRepositoryPermissionValues = map[RepositoryPermission]struct{}{ } // ValidateRepositoryPermission validates a given RepositoryPermission. -// Use as errs.Append(ValidateRepositoryPermission(permission), permission, "FieldName") +// Use as errs.Append(ValidateRepositoryPermission(permission), permission, "FieldName"). func ValidateRepositoryPermission(p RepositoryPermission) error { _, ok := knownRepositoryPermissionValues[p] if !ok { @@ -114,7 +117,7 @@ func ValidateRepositoryPermission(p RepositoryPermission) error { return nil } -// RepositoryPermissionVar returns a pointer to a RepositoryPermission +// RepositoryPermissionVar returns a pointer to a RepositoryPermission. func RepositoryPermissionVar(p RepositoryPermission) *RepositoryPermission { return &p } @@ -137,6 +140,7 @@ const ( ) // knownLicenseTemplateValues is a map of known LicenseTemplate values, used for validation +//nolint:gochecknoglobals var knownLicenseTemplateValues = map[LicenseTemplate]struct{}{ LicenseTemplateApache2: {}, LicenseTemplateMIT: {}, @@ -144,7 +148,7 @@ var knownLicenseTemplateValues = map[LicenseTemplate]struct{}{ } // ValidateLicenseTemplate validates a given LicenseTemplate. -// Use as errs.Append(ValidateLicenseTemplate(template), template, "FieldName") +// Use as errs.Append(ValidateLicenseTemplate(template), template, "FieldName"). func ValidateLicenseTemplate(t LicenseTemplate) error { _, ok := knownLicenseTemplateValues[t] if !ok { @@ -153,7 +157,7 @@ func ValidateLicenseTemplate(t LicenseTemplate) error { return nil } -// LicenseTemplateVar returns a pointer to a LicenseTemplate +// LicenseTemplateVar returns a pointer to a LicenseTemplate. func LicenseTemplateVar(t LicenseTemplate) *LicenseTemplate { return &t } diff --git a/gitprovider/errors.go b/gitprovider/errors.go index bf7ee56f..3fb55a47 100644 --- a/gitprovider/errors.go +++ b/gitprovider/errors.go @@ -23,58 +23,58 @@ import ( ) var ( - // ErrNoProviderSupport describes that the provider doesn't support the requested feature + // ErrNoProviderSupport describes that the provider doesn't support the requested feature. ErrNoProviderSupport = errors.New("no provider support for this feature") // ErrDomainUnsupported describes the case where e.g. a GitHub provider used for trying to get - // information from e.g. gitlab.com + // information from e.g. "gitlab.com". ErrDomainUnsupported = errors.New("the client doesn't support handling requests for this domain") // ErrNotTopLevelOrganization describes the case where it's mandatory to specify a top-level organization - // (e.g. to access teams), but a sub-organization was passed as the OrganizationRef + // (e.g. to access teams), but a sub-organization was passed as the OrganizationRef. ErrNotTopLevelOrganization = errors.New("expected top-level organization, received sub-organization instead") - // ErrInvalidArgument describes a generic error where an invalid argument have been specified to a function + // ErrInvalidArgument describes a generic error where an invalid argument have been specified to a function. ErrInvalidArgument = errors.New("invalid argument specified") - // ErrUnexpectedEvent describes a case where something really unexpected happened in the program + // ErrUnexpectedEvent describes a case where something really unexpected happened in the program. ErrUnexpectedEvent = errors.New("an unexpected error occurred") // ErrAlreadyExists is returned by .Create() requests if the given resource already exists. - // Use .Reconcile() instead if you want to idempotently create the resource + // Use .Reconcile() instead if you want to idempotently create the resource. ErrAlreadyExists = errors.New("resource already exists, cannot create object. Use Reconcile() to create it idempotently") - // ErrNotFound is returned by .Get() and .Update() calls if the given resource doesn't exist + // ErrNotFound is returned by .Get() and .Update() calls if the given resource doesn't exist. ErrNotFound = errors.New("the requested resource was not found") // ErrInvalidServerData is returned when the server returned invalid data, e.g. missing required fields in the response. ErrInvalidServerData = errors.New("got invalid data from server, don't know how to handle") - // ErrURLUnsupportedScheme is returned if an URL without the HTTPS scheme is parsed + // ErrURLUnsupportedScheme is returned if an URL without the HTTPS scheme is parsed. ErrURLUnsupportedScheme = errors.New("unsupported URL scheme, only HTTPS supported") - // ErrURLUnsupportedParts is returned if an URL with fragment, query values and/or user information is parsed + // ErrURLUnsupportedParts is returned if an URL with fragment, query values and/or user information is parsed. ErrURLUnsupportedParts = errors.New("URL cannot have fragments, query values nor user information") - // ErrURLInvalid is returned if an URL is invalid when parsing + // ErrURLInvalid is returned if an URL is invalid when parsing. ErrURLInvalid = errors.New("invalid organization, user or repository URL") - // ErrURLMissingRepoName is returned if there is no repository name in the URL + // ErrURLMissingRepoName is returned if there is no repository name in the URL. ErrURLMissingRepoName = errors.New("missing repository name") ) -// HTTPError is an error that contains context about the HTTP request/response that failed +// HTTPError is an error that contains context about the HTTP request/response that failed. type HTTPError struct { // HTTP response that caused this error. Response *http.Response `json:"-"` - // Full error message, human-friendly and formatted + // Full error message, human-friendly and formatted. ErrorMessage string `json:"errorMessage"` - // Message about what happened + // Message about what happened. Message string `json:"message"` - // Where to find more information about the error + // Where to find more information about the error. DocumentationURL string `json:"documentationURL"` } -// Error implements the error interface +// Error implements the error interface. func (e *HTTPError) Error() string { return e.ErrorMessage } -// RateLimitError is an error, extending HTTPError, that contains context about rate limits +// RateLimitError is an error, extending HTTPError, that contains context about rate limits. type RateLimitError struct { - // RateLimitError extends HTTPError + // RateLimitError extends HTTPError. HTTPError `json:",inline"` // The number of requests per hour the client is currently limited to. @@ -85,22 +85,22 @@ type RateLimitError struct { Reset time.Time `json:"reset"` } -// ValidationError is an error, extending HTTPError, that contains context about failed server-side validation +// ValidationError is an error, extending HTTPError, that contains context about failed server-side validation. type ValidationError struct { - // RateLimitError extends HTTPError + // RateLimitError extends HTTPError. HTTPError `json:",inline"` - // Errors contain context about what validation(s) failed + // Errors contain context about what validation(s) failed. Errors []ValidationErrorItem `json:"errors"` } -// ValidationErrorItem represents a single invalid field in an invalid request +// ValidationErrorItem represents a single invalid field in an invalid request. type ValidationErrorItem struct { - // Resource on which the error occurred + // Resource on which the error occurred. Resource string `json:"resource"` - // Field on which the error occurred + // Field on which the error occurred. Field string `json:"field"` - // Code for the validation error + // Code for the validation error. Code string `json:"code"` // Message describing the error. Errors with Code == "custom" will always have this set. Message string `json:"message"` @@ -111,6 +111,6 @@ type ValidationErrorItem struct { // "the login was successful but you don't have permission to access this resource". In that case, a // 404 Not Found error would be returned. type InvalidCredentialsError struct { - // InvalidCredentialsError extends HTTPError + // InvalidCredentialsError extends HTTPError. HTTPError `json:",inline"` } diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 454090d9..83552512 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -19,7 +19,7 @@ package gitprovider import "context" // ProviderID is a typed string for a given Git provider -// The provider constants are defined in their respective packages +// The provider constants are defined in their respective packages. type ProviderID string // CreatableInfo is an interface which all *Info objects that can be created @@ -29,7 +29,7 @@ type CreatableInfo interface { // Set (non-nil) and required fields should be validated. ValidateInfo() error // Default will be run after validation, setting optional pointer fields to their - // default values before doing the POST request + // default values before doing the POST request. Default() } @@ -71,9 +71,9 @@ type GenericReconcilable interface { Reconcile(ctx context.Context) (actionTaken bool, err error) } -// Object is the interface all types should implement +// Object is the interface all types should implement. type Object interface { - // APIObject returns the underlying value that was returned from the server + // APIObject returns the underlying value that was returned from the server. APIObject() interface{} } diff --git a/gitprovider/options.go b/gitprovider/options.go index 21c41c26..f0abd148 100644 --- a/gitprovider/options.go +++ b/gitprovider/options.go @@ -21,7 +21,7 @@ import "github.com/fluxcd/go-git-providers/validation" // MakeRepositoryCreateOptions returns a RepositoryCreateOptions based off the mutator functions // given to e.g. RepositoriesClient.Create(). The returned validation error may be ignored in the // case that the client allows e.g. other license templates than those that are common. -// validation.ErrFieldEnumInvalid is returned if the license template doesn't match known values +// validation.ErrFieldEnumInvalid is returned if the license template doesn't match known values. func MakeRepositoryCreateOptions(opts ...RepositoryCreateOption) (RepositoryCreateOptions, error) { o := &RepositoryCreateOptions{} for _, opt := range opts { @@ -31,36 +31,36 @@ func MakeRepositoryCreateOptions(opts ...RepositoryCreateOption) (RepositoryCrea return *o, o.ValidateInfo() } -// RepositoryCreateOptions implements CreatableInfo +// RepositoryCreateOptions implements CreatableInfo. var _ CreatableInfo = &RepositoryCreateOptions{} -// RepositoryReconcileOption is an interface for applying options to when reconciling repositories +// RepositoryReconcileOption is an interface for applying options to when reconciling repositories. type RepositoryReconcileOption interface { - // RepositoryCreateOption is embedded, as reconcile uses the create options + // RepositoryCreateOption is embedded, as reconcile uses the create options. RepositoryCreateOption } -// RepositoryCreateOption is an interface for applying options to when creating repositories +// RepositoryCreateOption is an interface for applying options to when creating repositories. type RepositoryCreateOption interface { - // ApplyToRepositoryCreateOptions should apply relevant options to the target + // ApplyToRepositoryCreateOptions should apply relevant options to the target. ApplyToRepositoryCreateOptions(target *RepositoryCreateOptions) } -// RepositoryCreateOptions specifies optional options when creating a repository +// RepositoryCreateOptions specifies optional options when creating a repository. type RepositoryCreateOptions struct { // AutoInit can be set to true in order to automatically initialize the Git repo with a // README.md and optionally a license in the first commit. // Default: nil (which means "false, don't create") AutoInit *bool - // LicenseTemplate lets the user specify a license template to use when AutoInit is true - // Default: nil - // Available options: See the LicenseTemplate enum + // LicenseTemplate lets the user specify a license template to use when AutoInit is true. + // Default: nil. + // Available options: See the LicenseTemplate enum. LicenseTemplate *LicenseTemplate } // ApplyToRepositoryCreateOptions applies the options defined in the options struct to the -// target struct that is being completed +// target struct that is being completed. func (opts *RepositoryCreateOptions) ApplyToRepositoryCreateOptions(target *RepositoryCreateOptions) { // Go through each field in opts, and apply it to target if set if opts.AutoInit != nil { @@ -72,10 +72,10 @@ func (opts *RepositoryCreateOptions) ApplyToRepositoryCreateOptions(target *Repo } // Default implements CreatableInfo, setting default values for the options if needed -// For this specific case, it's ok to leave things as nil +// For this specific case, it's ok to leave things as nil. func (opts *RepositoryCreateOptions) Default() {} -// ValidateInfo validates that the options are valid +// ValidateInfo validates that the options are valid. func (opts *RepositoryCreateOptions) ValidateInfo() error { errs := validation.New("RepositoryCreateOptions") if opts.LicenseTemplate != nil { diff --git a/gitprovider/repositoryref.go b/gitprovider/repositoryref.go index 4e2ded1a..cad689db 100644 --- a/gitprovider/repositoryref.go +++ b/gitprovider/repositoryref.go @@ -24,27 +24,25 @@ import ( "github.com/fluxcd/go-git-providers/validation" ) -// TODO: Add equality methods for IdentityRef and RepositoryRefs - -// IdentityType is a typed string for what kind of identity type an IdentityRef is +// IdentityType is a typed string for what kind of identity type an IdentityRef is. type IdentityType string const ( - // IdentityTypeUser represents an identity for a user account + // IdentityTypeUser represents an identity for a user account. IdentityTypeUser = IdentityType("user") - // IdentityTypeOrganization represents an identity for an organization + // IdentityTypeOrganization represents an identity for an organization. IdentityTypeOrganization = IdentityType("organization") - // IdentityTypeSuborganization represents an identity for a sub-organization + // IdentityTypeSuborganization represents an identity for a sub-organization. IdentityTypeSuborganization = IdentityType("suborganization") ) -// IdentityRef references an organization or user account in a Git provider +// IdentityRef references an organization or user account in a Git provider. type IdentityRef interface { - // IdentityRef implements ValidateTarget so it can easily be validated as a field + // IdentityRef implements ValidateTarget so it can easily be validated as a field. validation.ValidateTarget // GetDomain returns the URL-domain for the Git provider backend, - // e.g. "github.com" or "self-hosted-gitlab.com:6443" + // e.g. "github.com" or "self-hosted-gitlab.com:6443". GetDomain() string // GetIdentity returns the user account name or a slash-separated path of the @@ -57,7 +55,7 @@ type IdentityRef interface { // IdentityTypeSuborganization are returned, this IdentityRef can be casted to a OrganizationRef. GetType() IdentityType - // String returns the HTTPS URL, and implements fmt.Stringer + // String returns the HTTPS URL, and implements fmt.Stringer. String() string } @@ -85,7 +83,7 @@ type UserRef struct { UserLogin string `json:"userLogin"` } -// UserRef implements IdentityRef +// UserRef implements IdentityRef. var _ IdentityRef = UserRef{} // GetDomain returns the the domain part of the endpoint, can include port information. @@ -93,22 +91,22 @@ func (u UserRef) GetDomain() string { return u.Domain } -// GetIdentity returns the identity of this actor, which in this case is the user login name +// GetIdentity returns the identity of this actor, which in this case is the user login name. func (u UserRef) GetIdentity() string { return u.UserLogin } -// GetType marks this UserRef as being a IdentityTypeUser +// GetType marks this UserRef as being a IdentityTypeUser. func (u UserRef) GetType() IdentityType { return IdentityTypeUser } -// String returns the HTTPS URL to access the User +// String returns the HTTPS URL to access the User. func (u UserRef) String() string { return fmt.Sprintf("https://%s/%s", u.GetDomain(), u.GetIdentity()) } -// ValidateFields validates its own fields for a given validator +// ValidateFields validates its own fields for a given validator. func (u UserRef) ValidateFields(validator validation.Validator) { // Require the Domain and Organization to be set if len(u.Domain) == 0 { @@ -119,13 +117,13 @@ func (u UserRef) ValidateFields(validator validation.Validator) { } } -// OrganizationRef implements IdentityRef +// OrganizationRef implements IdentityRef. var _ IdentityRef = OrganizationRef{} -// OrganizationRef is an implementation of OrganizationRef +// OrganizationRef is an implementation of OrganizationRef. type OrganizationRef struct { // Domain returns e.g. "github.com", "gitlab.com" or a custom domain like "self-hosted-gitlab.com" (GitLab) - // The domain _might_ contain port information, in the form of "host:port", if applicable + // The domain _might_ contain port information, in the form of "host:port", if applicable. // +required Domain string `json:"domain"` @@ -145,13 +143,13 @@ func (o OrganizationRef) GetDomain() string { return o.Domain } -// GetIdentity returns the identity of this actor, which in this case is the user login name +// GetIdentity returns the identity of this actor, which in this case is the user login name. func (o OrganizationRef) GetIdentity() string { orgParts := append([]string{o.Organization}, o.SubOrganizations...) return strings.Join(orgParts, "/") } -// GetType marks this UserRef as being a IdentityTypeUser +// GetType marks this UserRef as being a IdentityTypeUser. func (o OrganizationRef) GetType() IdentityType { if len(o.SubOrganizations) > 0 { return IdentityTypeSuborganization @@ -159,12 +157,12 @@ func (o OrganizationRef) GetType() IdentityType { return IdentityTypeOrganization } -// String returns the HTTPS URL to access the Organization +// String returns the HTTPS URL to access the Organization. func (o OrganizationRef) String() string { return fmt.Sprintf("https://%s/%s", o.GetDomain(), o.GetIdentity()) } -// ValidateFields validates its own fields for a given validator +// ValidateFields validates its own fields for a given validator. func (o OrganizationRef) ValidateFields(validator validation.Validator) { // Require the Domain and Organization to be set if len(o.Domain) == 0 { @@ -181,7 +179,7 @@ type OrgRepositoryRef struct { OrganizationRef `json:",inline"` // RepositoryName specifies the Git repository name. This field is URL-friendly, - // e.g. "kubernetes" or "cluster-api-provider-aws" + // e.g. "kubernetes" or "cluster-api-provider-aws". // +required RepositoryName string `json:"repositoryName"` } @@ -196,7 +194,7 @@ func (r OrgRepositoryRef) GetRepository() string { return r.RepositoryName } -// ValidateFields validates its own fields for a given validator +// ValidateFields validates its own fields for a given validator. func (r OrgRepositoryRef) ValidateFields(validator validation.Validator) { // First, validate the embedded OrganizationRef r.OrganizationRef.ValidateFields(validator) @@ -206,7 +204,7 @@ func (r OrgRepositoryRef) ValidateFields(validator validation.Validator) { } } -// GetCloneURL gets the clone URL for the specified transport type +// GetCloneURL gets the clone URL for the specified transport type. func (r OrgRepositoryRef) GetCloneURL(transport TransportType) string { return GetCloneURL(r, transport) } @@ -217,12 +215,12 @@ type UserRepositoryRef struct { UserRef `json:",inline"` // RepositoryName specifies the Git repository name. This field is URL-friendly, - // e.g. "kubernetes" or "cluster-api-provider-aws" + // e.g. "kubernetes" or "cluster-api-provider-aws". // +required RepositoryName string `json:"repositoryName"` } -// String returns the HTTPS URL to access the repository +// String returns the HTTPS URL to access the repository. func (r UserRepositoryRef) String() string { return fmt.Sprintf("%s/%s", r.UserRef.String(), r.RepositoryName) } @@ -232,7 +230,7 @@ func (r UserRepositoryRef) GetRepository() string { return r.RepositoryName } -// ValidateFields validates its own fields for a given validator +// ValidateFields validates its own fields for a given validator. func (r UserRepositoryRef) ValidateFields(validator validation.Validator) { // First, validate the embedded OrganizationRef r.UserRef.ValidateFields(validator) @@ -242,7 +240,7 @@ func (r UserRepositoryRef) ValidateFields(validator validation.Validator) { } } -// GetCloneURL gets the clone URL for the specified transport type +// GetCloneURL gets the clone URL for the specified transport type. func (r UserRepositoryRef) GetCloneURL(transport TransportType) string { return GetCloneURL(r, transport) } @@ -261,7 +259,7 @@ func GetCloneURL(rs RepositoryRef, transport TransportType) string { return "" } -// ParseOrganizationURL parses an URL to an organization into a OrganizationRef object +// ParseOrganizationURL parses an URL to an organization into a OrganizationRef object. func ParseOrganizationURL(o string) (*OrganizationRef, error) { u, parts, err := parseURL(o) if err != nil { @@ -280,7 +278,7 @@ func ParseOrganizationURL(o string) (*OrganizationRef, error) { return info, nil } -// ParseUserURL parses an URL to an organization into a UserRef object +// ParseUserURL parses an URL to an organization into a UserRef object. func ParseUserURL(u string) (*UserRef, error) { // Use the same logic as for parsing organization URLs, but return an UserRef object orgInfoPtr, err := ParseOrganizationURL(u) @@ -294,7 +292,7 @@ func ParseUserURL(u string) (*UserRef, error) { return userRef, nil } -// ParseUserRepositoryURL parses a HTTPS clone URL into a UserRepositoryRef object +// ParseUserRepositoryURL parses a HTTPS clone URL into a UserRepositoryRef object. func ParseUserRepositoryURL(r string) (*UserRepositoryRef, error) { orgInfoPtr, repoName, err := parseRepositoryURL(r) if err != nil { @@ -312,7 +310,7 @@ func ParseUserRepositoryURL(r string) (*UserRepositoryRef, error) { }, nil } -// ParseOrgRepositoryURL parses a HTTPS clone URL into a OrgRepositoryRef object +// ParseOrgRepositoryURL parses a HTTPS clone URL into a OrgRepositoryRef object. func ParseOrgRepositoryURL(r string) (*OrgRepositoryRef, error) { orgInfoPtr, repoName, err := parseRepositoryURL(r) if err != nil { diff --git a/gitprovider/types_organization.go b/gitprovider/types_organization.go index fa972b9c..549d858d 100644 --- a/gitprovider/types_organization.go +++ b/gitprovider/types_organization.go @@ -16,12 +16,12 @@ limitations under the License. package gitprovider -// OrganizationInfo represents an (top-level- or sub-) organization +// OrganizationInfo represents an (top-level- or sub-) organization. type OrganizationInfo struct { - // Name is the human-friendly name of this organization, e.g. "Flux" or "Kubernetes SIGs" + // Name is the human-friendly name of this organization, e.g. "Flux" or "Kubernetes SIGs". Name *string `json:"name"` - // Description returns a description for the organization + // Description returns a description for the organization. Description *string `json:"description"` } diff --git a/gitprovider/types_repository.go b/gitprovider/types_repository.go index 6161ee86..cf96ec98 100644 --- a/gitprovider/types_repository.go +++ b/gitprovider/types_repository.go @@ -21,41 +21,42 @@ import ( ) const ( - // the default repository visibility is private + // the default repository visibility is private. defaultRepositoryVisibility = RepositoryVisibilityPrivate - // the default repository permission is "pull" (or read) + // the default repository permission is "pull" (or read). defaultRepoPermission = RepositoryPermissionPull // the default branch name. // TODO: When enough Git providers support setting this at both POST and PATCH-time - // (including when auto-initing), change this to "main" + // (including when auto-initing), change this to "main". defaultBranchName = "master" - // by default, deploy keys are read-only + // by default, deploy keys are read-only. defaultDeployKeyReadOnly = true ) +// RepositoryInfo implements CreatableInfo. var _ CreatableInfo = &RepositoryInfo{} -// RepositoryInfo represents a Git repository provided by a Git provider +// RepositoryInfo represents a Git repository provided by a Git provider. type RepositoryInfo struct { - // Description returns a description for the repository - // No default value at POST-time + // Description returns a description for the repository. + // No default value at POST-time. // +optional Description *string `json:"description"` // DefaultBranch describes the default branch for the given repository. This has // historically been "master" (and is as of writing still the Git default), but is // expected to be changed to e.g. "main" shortly in the future. - // Default value at POST-time: master (but this can and will change in future library versions!) + // Default value at POST-time: master (but this can and will change in future library versions!). // +optional DefaultBranch *string `json:"defaultBranch"` - // Visibility returns the desired visibility for the repository - // Default value at POST-time: RepositoryVisibilityPrivate + // Visibility returns the desired visibility for the repository. + // Default value at POST-time: RepositoryVisibilityPrivate. // +optional Visibility *RepositoryVisibility `json:"visibility"` } -// Default defaults the Repository, implementing the CreatableInfo interface +// Default defaults the Repository, implementing the CreatableInfo interface. func (r *RepositoryInfo) Default() { if r.Visibility == nil { r.Visibility = RepositoryVisibilityVar(defaultRepositoryVisibility) @@ -65,7 +66,7 @@ func (r *RepositoryInfo) Default() { } } -// ValidateInfo validates the object at {Object}.Set() and POST-time +// ValidateInfo validates the object at {Object}.Set() and POST-time. func (r *RepositoryInfo) ValidateInfo() error { validator := validation.New("Repository") // Validate the Visibility enum @@ -75,31 +76,30 @@ func (r *RepositoryInfo) ValidateInfo() error { return validator.Error() } -// TeamAccess implements Object and RepositoryRef interfaces -// TeamAccess can be created and deleted +// TeamAccessInfo implements CreatableInfo. var _ CreatableInfo = &TeamAccessInfo{} // TeamAccessInfo contains high-level information about a team's access to a repository. type TeamAccessInfo struct { - // Name describes the name of the team. The team name may contain slashes + // Name describes the name of the team. The team name may contain slashes. // +required Name string `json:"name"` - // Permission describes the permission level for which the team is allowed to operate - // Default: pull - // Available options: See the RepositoryPermission enum + // Permission describes the permission level for which the team is allowed to operate. + // Default: pull. + // Available options: See the RepositoryPermission enum. // +optional Permission *RepositoryPermission `json:"permission,omitempty"` } -// Default defaults the TeamAccess, implementing the CreatableInfo interface +// Default defaults the TeamAccess fields. func (ta *TeamAccessInfo) Default() { if ta.Permission == nil { ta.Permission = RepositoryPermissionVar(defaultRepoPermission) } } -// ValidateInfo validates the object at {Object}.Set() and POST-time +// ValidateInfo validates the object at {Object}.Set() and POST-time. func (ta *TeamAccessInfo) ValidateInfo() error { validator := validation.New("TeamAccess") // Make sure we've set the name of the team @@ -113,32 +113,33 @@ func (ta *TeamAccessInfo) ValidateInfo() error { return validator.Error() } +// DeployKeyInfo implements CreatableInfo. var _ CreatableInfo = &DeployKeyInfo{} // DeployKeyInfo contains high-level information about a deploy key. type DeployKeyInfo struct { - // Name is the human-friendly interpretation of what the key is for (and does) + // Name is the human-friendly interpretation of what the key is for (and does). // +required Name string `json:"name"` - // Key specifies the public part of the deploy (e.g. SSH) key + // Key specifies the public part of the deploy (e.g. SSH) key. // +required Key []byte `json:"key"` - // ReadOnly specifies whether this DeployKey can write to the repository or not - // Default value at POST-time: true + // ReadOnly specifies whether this DeployKey can write to the repository or not. + // Default value at POST-time: true. // +optional ReadOnly *bool `json:"readOnly,omitempty"` } -// Default defaults the DeployKey, implementing the CreatableInfo interface +// Default defaults the DeployKey fields. func (dk *DeployKeyInfo) Default() { if dk.ReadOnly == nil { dk.ReadOnly = BoolVar(defaultDeployKeyReadOnly) } } -// ValidateInfo validates the object at {Object}.Set() and POST-time +// ValidateInfo validates the object at {Object}.Set() and POST-time. func (dk *DeployKeyInfo) ValidateInfo() error { validator := validation.New("DeployKey") // Make sure we've set the name of the deploy key diff --git a/gitprovider/util.go b/gitprovider/util.go index 431e52f8..12c6d935 100644 --- a/gitprovider/util.go +++ b/gitprovider/util.go @@ -16,12 +16,12 @@ limitations under the License. package gitprovider -// BoolVar returns a pointer to the given bool +// BoolVar returns a pointer to the given bool. func BoolVar(b bool) *bool { return &b } -// StringVar returns a pointer to the given string +// StringVar returns a pointer to the given string. func StringVar(s string) *string { return &s } diff --git a/validation/multierror.go b/validation/multierror.go index 97ecd28e..75513e8b 100644 --- a/validation/multierror.go +++ b/validation/multierror.go @@ -59,8 +59,8 @@ type MultiError struct { Errors []error } -// Error implements the error interface on the pointer type of MultiError.Error -// This enforces callers to always return &MultiError{} for consistency +// Error implements the error interface on the pointer type of MultiError.Error. +// This enforces callers to always return &MultiError{} for consistency. func (e *MultiError) Error() string { errStr := "" for _, err := range e.Errors { @@ -70,7 +70,7 @@ func (e *MultiError) Error() string { } // Is implements the interface used by errors.Is in order to check if two errors are the same. -// This function recursively checks all contained errors +// This function recursively checks all contained errors. func (e *MultiError) Is(target error) bool { // If target is a MultiError, return that target is a match _, ok := target.(*MultiError) @@ -88,7 +88,7 @@ func (e *MultiError) Is(target error) bool { } // As implements the interface used by errors.As in order to get the value of an embedded -// struct error of this MultiError +// struct error of this MultiError. func (e *MultiError) As(target interface{}) bool { // There is no need to check for if target is a MultiError, as it it would be, this function // wouldn't be called. @@ -104,13 +104,14 @@ func (e *MultiError) As(target interface{}) bool { // disallowedCompareAsErrorNames contains a list of which errors should NOT be compared for equality // using errors.As, as they could be very different errors although being the same type +//nolint:gochecknoglobals var disallowedCompareAsErrorNames = map[string]struct{}{ "*errors.errorString": {}, "*fmt.wrapError": {}, } // TestExpectErrors loops through all expected errors and make sure that errors.Is returns true -// for all of them. If there aren't any expected errors, and err != nil, an error will be reported too +// for all of them. If there aren't any expected errors, and err != nil, an error will be reported too. func TestExpectErrors(t testing.TB, funcName string, err error, expectedErrs ...error) { for _, expectedErr := range expectedErrs { // Check equality between the errors using errors.Is diff --git a/validation/validation.go b/validation/validation.go index 0cd745df..83d78f05 100644 --- a/validation/validation.go +++ b/validation/validation.go @@ -23,11 +23,11 @@ import ( ) var ( - // ErrFieldRequired specifies the case where a required field isn't populated at use time + // ErrFieldRequired specifies the case where a required field isn't populated at use time. ErrFieldRequired = errors.New("field is required") - // ErrFieldInvalid specifies the case where a field isn't populated in a valid manner + // ErrFieldInvalid specifies the case where a field isn't populated in a valid manner. ErrFieldInvalid = errors.New("field is invalid") - // ErrFieldEnumInvalid specifies the case where the given value isn't part of the known values in the enum + // ErrFieldEnumInvalid specifies the case where the given value isn't part of the known values in the enum. ErrFieldEnumInvalid = errors.New("field value isn't known to this enum") ) @@ -62,7 +62,7 @@ type ValidateTarget interface { ValidateFields(v Validator) } -// New creates a new validator struct for the given struct name +// New creates a new validator struct for the given struct name. func New(name string) Validator { return &validator{name, nil} } @@ -98,7 +98,7 @@ func (v *validator) Required(fieldPaths ...string) { // Invalid is a helper method for Append, registering ErrFieldInvalid as the cause, along with what field // caused the error. fieldPaths should contain the names of all nested sub-fields (of the struct) that caused -// the error. Specifying the value that was invalid is also supported +// the error. Specifying the value that was invalid is also supported. func (v *validator) Invalid(value interface{}, fieldPaths ...string) { v.Append(ErrFieldInvalid, value, fieldPaths...) } @@ -125,7 +125,7 @@ func (v *validator) Append(err error, value interface{}, fieldPaths ...string) { // Error returns an aggregated error (or nil), based on the errors that have been registered // A *MultiError is returned if there are multiple errors. Users of this function might use // multiErr := &MultiError{}; errors.As(err, &multiErr) or errors.Is(err, multiErr) to detect -// that many errors were returned +// that many errors were returned. func (v *validator) Error() error { // If there aren't any errors in the list, return nil quickly if len(v.errs) == 0 { From 3201aaef7516eb84dd24f27f37391b8866b59ea0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 13:59:36 +0300 Subject: [PATCH 13/19] Strip the "generic" prefix, not needed --- gitprovider/gitprovider.go | 12 ++++++------ gitprovider/resources.go | 18 +++++++++--------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 83552512..2367665e 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -33,9 +33,9 @@ type CreatableInfo interface { Default() } -// GenericUpdatable is an interface which all objects that can be updated +// Updatable is an interface which all objects that can be updated // using the Client implement. -type GenericUpdatable interface { +type Updatable interface { // Update will apply the desired state in this object to the server. // Only set fields will be respected (i.e. PATCH behaviour). // In order to apply changes to this object, use the .Set({Resource}Info) error @@ -48,18 +48,18 @@ type GenericUpdatable interface { Update(ctx context.Context) error } -// GenericDeletable is an interface which all objects that can be deleted +// Deletable is an interface which all objects that can be deleted // using the Client implement. -type GenericDeletable interface { +type Deletable interface { // Delete deletes the current resource irreversible. // // ErrNotFound is returned if the resource doesn't exist anymore. Delete(ctx context.Context) error } -// GenericReconcilable is an interface which all objects that can be reconciled +// Reconcilable is an interface which all objects that can be reconciled // using the Client implement. -type GenericReconcilable interface { +type Reconcilable interface { // Reconcile makes sure the desired state in this object (called "req" here) becomes // the actual state in the backing Git provider. // diff --git a/gitprovider/resources.go b/gitprovider/resources.go index 34a66695..fb3d9218 100644 --- a/gitprovider/resources.go +++ b/gitprovider/resources.go @@ -51,11 +51,11 @@ type UserRepository interface { // allowing access to the underlying object returned from the API. Object // The repository can be updated. - GenericUpdatable + Updatable // The repository can be reconciled. - GenericReconcilable + Reconcilable // The repository can be deleted. - GenericDeletable + Deletable // RepositoryBound returns repository reference details. RepositoryBound @@ -84,11 +84,11 @@ type DeployKey interface { // allowing access to the underlying object returned from the API. Object // The deploy key can be updated. - GenericUpdatable + Updatable // The deploy key can be reconciled. - GenericReconcilable + Reconcilable // The deploy key can be deleted. - GenericDeletable + Deletable // RepositoryBound returns repository reference details. RepositoryBound @@ -105,11 +105,11 @@ type TeamAccess interface { // allowing access to the underlying object returned from the API. Object // The deploy key can be updated. - GenericUpdatable + Updatable // The deploy key can be reconciled. - GenericReconcilable + Reconcilable // The deploy key can be deleted. - GenericDeletable + Deletable // RepositoryBound returns repository reference details. RepositoryBound From 45d37bc63c1f8c490fc5930ce3133546905c6184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 15:15:16 +0300 Subject: [PATCH 14/19] Move unit test for getPermissionFromMap --- github/resource_teamaccess_test.go | 109 +++++++++++++++++++++++++++++ github/util_test.go | 74 -------------------- 2 files changed, 109 insertions(+), 74 deletions(-) create mode 100644 github/resource_teamaccess_test.go diff --git a/github/resource_teamaccess_test.go b/github/resource_teamaccess_test.go new file mode 100644 index 00000000..b5e9b8f3 --- /dev/null +++ b/github/resource_teamaccess_test.go @@ -0,0 +1,109 @@ +/* +Copyright 2020 The Flux CD contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package github + +import ( + "reflect" + "testing" + + "github.com/fluxcd/go-git-providers/gitprovider" +) + +func Test_getPermissionFromMap(t *testing.T) { + tests := []struct { + name string + permissions map[string]bool + want *gitprovider.RepositoryPermission + }{ + { + name: "pull", + permissions: map[string]bool{ + "pull": true, + "triage": false, + "push": false, + "maintain": false, + "admin": false, + }, + want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionPull), + }, + { + name: "push", + permissions: map[string]bool{ + "triage": false, + "push": true, + "maintain": false, + "pull": true, + "admin": false, + }, + want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionPush), + }, + { + name: "admin", + permissions: map[string]bool{ + "admin": true, + "pull": true, + "triage": true, + "maintain": true, + "push": true, + }, + want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionAdmin), + }, + { + name: "none", + permissions: map[string]bool{ + "admin": false, + "pull": false, + "push": false, + "maintain": false, + "triage": false, + }, + want: nil, + }, + { + name: "false data", + permissions: map[string]bool{ + "pull": false, + "triage": false, + "push": false, + "maintain": false, + "admin": false, + "invalid": true, + }, + want: nil, + }, + { + name: "not all specifed", + permissions: map[string]bool{ + "pull": false, + "triage": false, + "push": false, + "maintain": false, + "admin": false, + "invalid": true, + }, + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotPermission := getPermissionFromMap(tt.permissions) + if !reflect.DeepEqual(gotPermission, tt.want) { + t.Errorf("getPermissionFromMap() = %v, want %v", gotPermission, tt.want) + } + }) + } +} diff --git a/github/util_test.go b/github/util_test.go index 3b07fd6e..6bbba362 100644 --- a/github/util_test.go +++ b/github/util_test.go @@ -19,7 +19,6 @@ package github import ( "net/http" "net/url" - "reflect" "testing" "github.com/fluxcd/go-git-providers/gitprovider" @@ -27,79 +26,6 @@ import ( "github.com/google/go-github/v32/github" ) -func Test_getPermissionFromMap(t *testing.T) { - tests := []struct { - name string - permissions map[string]bool - want *gitprovider.RepositoryPermission - }{ - { - name: "pull", - permissions: map[string]bool{ - "pull": true, - "triage": false, - "push": false, - "maintain": false, - "admin": false, - }, - want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionPull), - }, - { - name: "push", - permissions: map[string]bool{ - "pull": true, - "triage": false, - "push": true, - "maintain": false, - "admin": false, - }, - want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionPush), - }, - { - name: "admin", - permissions: map[string]bool{ - "pull": true, - "triage": true, - "push": true, - "maintain": true, - "admin": true, - }, - want: gitprovider.RepositoryPermissionVar(gitprovider.RepositoryPermissionAdmin), - }, - { - name: "none", - permissions: map[string]bool{ - "pull": false, - "triage": false, - "push": false, - "maintain": false, - "admin": false, - }, - want: nil, - }, - { - name: "false data", - permissions: map[string]bool{ - "pull": false, - "triage": false, - "push": false, - "maintain": false, - "admin": false, - "invalid": true, - }, - want: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - gotPermission := getPermissionFromMap(tt.permissions) - if !reflect.DeepEqual(gotPermission, tt.want) { - t.Errorf("getPermissionFromMap() = %v, want %v", gotPermission, tt.want) - } - }) - } -} - func Test_validateAPIObject(t *testing.T) { tests := []struct { name string From a8239249eb6e122a4f34a17856dc95122b42d2ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 15:22:15 +0300 Subject: [PATCH 15/19] Create helper method for validation + defaulting for Create + Reconcile, and unit-test this. --- github/client_repositories_org.go | 13 +++--- github/client_repositories_user.go | 5 ++- github/client_repository_deploykey.go | 11 ++--- github/client_repository_teamaccess.go | 11 ++--- gitprovider/gitprovider.go | 16 +++++++ gitprovider/gitprovider_test.go | 59 ++++++++++++++++++++++++++ gitprovider/options.go | 3 +- 7 files changed, 98 insertions(+), 20 deletions(-) create mode 100644 gitprovider/gitprovider_test.go diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go index 73988e15..de6664be 100644 --- a/github/client_repositories_org.go +++ b/github/client_repositories_org.go @@ -109,8 +109,9 @@ func (c *OrgRepositoriesClient) Create(ctx context.Context, ref gitprovider.OrgR // If req doesn't equal the actual state, the resource will be updated (actionTaken == true). // If req is already the actual state, this is a no-op (actionTaken == false). func (c *OrgRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.OrgRepositoryRef, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryReconcileOption) (gitprovider.OrgRepository, bool, error) { - // First thing, validate the request - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, false, err } @@ -137,17 +138,17 @@ func getRepository(ctx context.Context, c *github.Client, ref gitprovider.Reposi } func createRepository(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, orgName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*github.Repository, error) { - // Make sure the request is valid - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, err } + // Assemble the options struct based on the given options o, err := gitprovider.MakeRepositoryCreateOptions(opts...) if err != nil { return nil, err } - // Default the request object - req.Default() // Convert to the API object and apply the options data := repositoryToAPI(&req, ref) diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go index 5cd0d9ff..16f0b063 100644 --- a/github/client_repositories_user.go +++ b/github/client_repositories_user.go @@ -112,8 +112,9 @@ func (c *UserRepositoriesClient) Create(ctx context.Context, // If req doesn't equal the actual state, the resource will be updated (actionTaken == true). // If req is already the actual state, this is a no-op (actionTaken == false). func (c *UserRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.UserRepositoryRef, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryReconcileOption) (gitprovider.UserRepository, bool, error) { - // First thing, validate the request - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, false, err } diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index 11f0ad1f..4a833aec 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -117,8 +117,9 @@ func (c *DeployKeyClient) Create(ctx context.Context, req gitprovider.DeployKeyI // If req doesn't equal the actual state, the resource will be deleted and recreated (actionTaken == true). // If req is already the actual state, this is a no-op (actionTaken == false). func (c *DeployKeyClient) Reconcile(ctx context.Context, req gitprovider.DeployKeyInfo) (gitprovider.DeployKey, bool, error) { - // First thing, validate the request - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, false, err } @@ -149,11 +150,11 @@ func (c *DeployKeyClient) Reconcile(ctx context.Context, req gitprovider.DeployK } func createDeployKey(ctx context.Context, c *github.Client, ref gitprovider.RepositoryRef, req gitprovider.DeployKeyInfo) (*github.Key, error) { - // Validate the create request and default - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, err } - req.Default() return createDeployKeyData(ctx, c, ref, deployKeyToAPI(&req)) } diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go index bd832dcd..2f503d0f 100644 --- a/github/client_repository_teamaccess.go +++ b/github/client_repository_teamaccess.go @@ -96,11 +96,11 @@ func (c *TeamAccessClient) List(ctx context.Context) ([]gitprovider.TeamAccess, // // ErrAlreadyExists will be returned if the resource already exists. func (c *TeamAccessClient) Create(ctx context.Context, req gitprovider.TeamAccessInfo) (gitprovider.TeamAccess, error) { - // Validate the request and default - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, err } - req.Default() opts := &github.TeamAddTeamRepoOptions{ Permission: string(*req.Permission), @@ -122,8 +122,9 @@ func (c *TeamAccessClient) Create(ctx context.Context, req gitprovider.TeamAcces func (c *TeamAccessClient) Reconcile(ctx context.Context, req gitprovider.TeamAccessInfo, ) (gitprovider.TeamAccess, bool, error) { - // First thing, validate the request - if err := req.ValidateInfo(); err != nil { + // First thing, validate and default the request to ensure a valid and fully-populated object + // (to minimize any possible diffs between desired and actual state) + if err := gitprovider.ValidateAndDefaultInfo(&req); err != nil { return nil, false, err } diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 2367665e..1966d0aa 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -88,3 +88,19 @@ type RepositoryBound interface { // Repository returns the RepositoryRef associated with this object. Repository() RepositoryRef } + +// ValidateAndDefaultInfo can be used in client Create() and Reconcile() functions, where the +// request object, which implements CreatableInfo, shall be first validated, and then defaulted. +// Defaulting happens at Create(), because we want to consistently apply this library's defaults +// across providers. Defaulting also happens at Reconcile(), because as the object has been created +// with defaults, the actual state fetched from the server will contain those defaults, and would +// result in a diff between the (possibly non-defaulted) request and actual state. +// TODO: Unit and integration test this. +// TODO: Document in Create() and Reconcile() that req is modified (?) and should not be used anymore. +func ValidateAndDefaultInfo(info CreatableInfo) error { + if err := info.ValidateInfo(); err != nil { + return err + } + info.Default() + return nil +} diff --git a/gitprovider/gitprovider_test.go b/gitprovider/gitprovider_test.go new file mode 100644 index 00000000..4e7b5701 --- /dev/null +++ b/gitprovider/gitprovider_test.go @@ -0,0 +1,59 @@ +/* +Copyright 2020 The Flux CD contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gitprovider + +import ( + "reflect" + "testing" +) + +func TestValidateAndDefaultInfo(t *testing.T) { + tests := []struct { + name string + info CreatableInfo + expected CreatableInfo + wantErr bool + }{ + { + name: "valid => defaulting", + info: &TeamAccessInfo{ + Name: "foo", + }, + expected: &TeamAccessInfo{ + Name: "foo", + Permission: RepositoryPermissionVar(defaultRepoPermission), + }, + wantErr: false, + }, + { + name: "invalid => no defaulting + error", + info: &TeamAccessInfo{}, + expected: &TeamAccessInfo{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ValidateAndDefaultInfo(tt.info); (err != nil) != tt.wantErr { + t.Errorf("ValidateAndDefaultInfo() error = %v, wantErr %v", err, tt.wantErr) + } + if !reflect.DeepEqual(tt.info, tt.expected) { + t.Errorf("ValidateAndDefaultInfo() object = %v, wanted %v", tt.info, tt.expected) + } + }) + } +} diff --git a/gitprovider/options.go b/gitprovider/options.go index f0abd148..96da58af 100644 --- a/gitprovider/options.go +++ b/gitprovider/options.go @@ -27,8 +27,7 @@ func MakeRepositoryCreateOptions(opts ...RepositoryCreateOption) (RepositoryCrea for _, opt := range opts { opt.ApplyToRepositoryCreateOptions(o) } - o.Default() - return *o, o.ValidateInfo() + return *o, ValidateAndDefaultInfo(o) } // RepositoryCreateOptions implements CreatableInfo. From 4e207eb1635de32c3fc2e20f054e6d83d05d626b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 15:54:48 +0300 Subject: [PATCH 16/19] Rename CreatableInfo => {Defaulted,}InfoRequest, and add an equality method to compare info objects --- github/client_repositories_org.go | 16 ++++------ github/client_repository_deploykey.go | 3 +- github/client_repository_teamaccess.go | 3 +- github/resource_teamaccess.go | 3 +- gitprovider/gitprovider.go | 27 ++++++++++++---- gitprovider/gitprovider_test.go | 4 +-- gitprovider/options.go | 15 +++------ gitprovider/types_defaulting_test.go | 4 +-- gitprovider/types_repository.go | 43 ++++++++++++++++++++------ 9 files changed, 72 insertions(+), 46 deletions(-) diff --git a/github/client_repositories_org.go b/github/client_repositories_org.go index de6664be..04dca56f 100644 --- a/github/client_repositories_org.go +++ b/github/client_repositories_org.go @@ -19,7 +19,6 @@ package github import ( "context" "errors" - "reflect" "github.com/google/go-github/v32/github" @@ -165,20 +164,17 @@ func createRepositoryData(ctx context.Context, c *github.Client, orgName string, return validateRepositoryAPIResp(apiObj, err) } -func reconcileRepository(ctx context.Context, actual gitprovider.UserRepository, req gitprovider.RepositoryInfo) (actionTaken bool, err error) { +func reconcileRepository(ctx context.Context, actual gitprovider.UserRepository, req gitprovider.RepositoryInfo) (bool, error) { // If the desired matches the actual state, just return the actual state - if reflect.DeepEqual(req, actual.Get()) { - return + if req.Equals(actual.Get()) { + return false, nil } - // Populate the desired state to the current-actual object - if err = actual.Set(req); err != nil { - return + if err := actual.Set(req); err != nil { + return false, err } // Apply the desired state by running Update - err = actual.Update(ctx) - actionTaken = true - return + return true, actual.Update(ctx) } func toCreateOpts(opts ...gitprovider.RepositoryReconcileOption) []gitprovider.RepositoryCreateOption { diff --git a/github/client_repository_deploykey.go b/github/client_repository_deploykey.go index 4a833aec..24b87f0c 100644 --- a/github/client_repository_deploykey.go +++ b/github/client_repository_deploykey.go @@ -19,7 +19,6 @@ package github import ( "context" "errors" - "reflect" "github.com/google/go-github/v32/github" @@ -137,7 +136,7 @@ func (c *DeployKeyClient) Reconcile(ctx context.Context, req gitprovider.DeployK } // If the desired matches the actual state, just return the actual state - if reflect.DeepEqual(req, actual.Get()) { + if req.Equals(actual.Get()) { return actual, false, nil } diff --git a/github/client_repository_teamaccess.go b/github/client_repository_teamaccess.go index 2f503d0f..97b3a437 100644 --- a/github/client_repository_teamaccess.go +++ b/github/client_repository_teamaccess.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "reflect" "github.com/google/go-github/v32/github" @@ -141,7 +140,7 @@ func (c *TeamAccessClient) Reconcile(ctx context.Context, } // If the desired matches the actual state, just return the actual state - if reflect.DeepEqual(req, actual.Get()) { + if req.Equals(actual.Get()) { return actual, false, nil } diff --git a/github/resource_teamaccess.go b/github/resource_teamaccess.go index e0ba3292..3fd6c386 100644 --- a/github/resource_teamaccess.go +++ b/github/resource_teamaccess.go @@ -19,7 +19,6 @@ package github import ( "context" "errors" - "reflect" "github.com/google/go-github/v32/github" @@ -107,7 +106,7 @@ func (ta *teamAccess) Reconcile(ctx context.Context) (bool, error) { } // If the desired matches the actual state, just return the actual state - if reflect.DeepEqual(req, actual.Get()) { + if req.Equals(actual.Get()) { return false, nil } diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 1966d0aa..54e315e6 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -22,14 +22,29 @@ import "context" // The provider constants are defined in their respective packages. type ProviderID string -// CreatableInfo is an interface which all *Info objects that can be created -// (using the Client) should implement. -type CreatableInfo interface { +// InfoRequest is an interface which all {Object}Info objects that can be used as Create() or Reconcile() +// requests in the Client should implement. Most likely, the struct should also implement DefaultedInfoRequest, +// as most objects have optional, defaulted fields. +type InfoRequest interface { // ValidateInfo validates the object at {Object}.Set() and POST-time, before defaulting. // Set (non-nil) and required fields should be validated. ValidateInfo() error + + // Equals can be used to check if this *Info request (the desired state) matches the actual + // passed in as the argument. + Equals(actual InfoRequest) bool +} + +// DefaultedInfoRequest is a superset of InfoRequest, also including a Default() function that can +// modify the underlying object, adding default values. ValidateAndDefaultInfo() can be used +// to first validate, and then default. +type DefaultedInfoRequest interface { + // DefaultedInfoRequest is a superset of InfoRequest + InfoRequest + // Default will be run after validation, setting optional pointer fields to their - // default values before doing the POST request. + // default values before doing the POST request. This function MUST be registered with + // the base struct as a pointer receiver. Default() } @@ -90,14 +105,14 @@ type RepositoryBound interface { } // ValidateAndDefaultInfo can be used in client Create() and Reconcile() functions, where the -// request object, which implements CreatableInfo, shall be first validated, and then defaulted. +// request object, which implements InfoRequest, shall be first validated, and then defaulted. // Defaulting happens at Create(), because we want to consistently apply this library's defaults // across providers. Defaulting also happens at Reconcile(), because as the object has been created // with defaults, the actual state fetched from the server will contain those defaults, and would // result in a diff between the (possibly non-defaulted) request and actual state. // TODO: Unit and integration test this. // TODO: Document in Create() and Reconcile() that req is modified (?) and should not be used anymore. -func ValidateAndDefaultInfo(info CreatableInfo) error { +func ValidateAndDefaultInfo(info DefaultedInfoRequest) error { if err := info.ValidateInfo(); err != nil { return err } diff --git a/gitprovider/gitprovider_test.go b/gitprovider/gitprovider_test.go index 4e7b5701..690affaf 100644 --- a/gitprovider/gitprovider_test.go +++ b/gitprovider/gitprovider_test.go @@ -24,8 +24,8 @@ import ( func TestValidateAndDefaultInfo(t *testing.T) { tests := []struct { name string - info CreatableInfo - expected CreatableInfo + info DefaultedInfoRequest + expected DefaultedInfoRequest wantErr bool }{ { diff --git a/gitprovider/options.go b/gitprovider/options.go index 96da58af..f5fd5643 100644 --- a/gitprovider/options.go +++ b/gitprovider/options.go @@ -16,7 +16,9 @@ limitations under the License. package gitprovider -import "github.com/fluxcd/go-git-providers/validation" +import ( + "github.com/fluxcd/go-git-providers/validation" +) // MakeRepositoryCreateOptions returns a RepositoryCreateOptions based off the mutator functions // given to e.g. RepositoriesClient.Create(). The returned validation error may be ignored in the @@ -27,12 +29,9 @@ func MakeRepositoryCreateOptions(opts ...RepositoryCreateOption) (RepositoryCrea for _, opt := range opts { opt.ApplyToRepositoryCreateOptions(o) } - return *o, ValidateAndDefaultInfo(o) + return *o, o.ValidateOptions() } -// RepositoryCreateOptions implements CreatableInfo. -var _ CreatableInfo = &RepositoryCreateOptions{} - // RepositoryReconcileOption is an interface for applying options to when reconciling repositories. type RepositoryReconcileOption interface { // RepositoryCreateOption is embedded, as reconcile uses the create options. @@ -70,12 +69,8 @@ func (opts *RepositoryCreateOptions) ApplyToRepositoryCreateOptions(target *Repo } } -// Default implements CreatableInfo, setting default values for the options if needed -// For this specific case, it's ok to leave things as nil. -func (opts *RepositoryCreateOptions) Default() {} - // ValidateInfo validates that the options are valid. -func (opts *RepositoryCreateOptions) ValidateInfo() error { +func (opts *RepositoryCreateOptions) ValidateOptions() error { errs := validation.New("RepositoryCreateOptions") if opts.LicenseTemplate != nil { errs.Append(ValidateLicenseTemplate(*opts.LicenseTemplate), *opts.LicenseTemplate, "LicenseTemplate") diff --git a/gitprovider/types_defaulting_test.go b/gitprovider/types_defaulting_test.go index 1dcd49b4..efd8d3f4 100644 --- a/gitprovider/types_defaulting_test.go +++ b/gitprovider/types_defaulting_test.go @@ -25,8 +25,8 @@ func TestDefaulting(t *testing.T) { tests := []struct { name string structName string - object CreatableInfo - expected CreatableInfo + object DefaultedInfoRequest + expected DefaultedInfoRequest }{ { name: "DeployKey: empty", diff --git a/gitprovider/types_repository.go b/gitprovider/types_repository.go index cf96ec98..4461bcc3 100644 --- a/gitprovider/types_repository.go +++ b/gitprovider/types_repository.go @@ -17,6 +17,8 @@ limitations under the License. package gitprovider import ( + "reflect" + "github.com/fluxcd/go-git-providers/validation" ) @@ -33,8 +35,9 @@ const ( defaultDeployKeyReadOnly = true ) -// RepositoryInfo implements CreatableInfo. -var _ CreatableInfo = &RepositoryInfo{} +// RepositoryInfo implements InfoRequest and DefaultedInfoRequest (with a pointer receiver). +var _ InfoRequest = RepositoryInfo{} +var _ DefaultedInfoRequest = &RepositoryInfo{} // RepositoryInfo represents a Git repository provided by a Git provider. type RepositoryInfo struct { @@ -56,7 +59,7 @@ type RepositoryInfo struct { Visibility *RepositoryVisibility `json:"visibility"` } -// Default defaults the Repository, implementing the CreatableInfo interface. +// Default defaults the Repository, implementing the InfoRequest interface. func (r *RepositoryInfo) Default() { if r.Visibility == nil { r.Visibility = RepositoryVisibilityVar(defaultRepositoryVisibility) @@ -67,7 +70,7 @@ func (r *RepositoryInfo) Default() { } // ValidateInfo validates the object at {Object}.Set() and POST-time. -func (r *RepositoryInfo) ValidateInfo() error { +func (r RepositoryInfo) ValidateInfo() error { validator := validation.New("Repository") // Validate the Visibility enum if r.Visibility != nil { @@ -76,8 +79,15 @@ func (r *RepositoryInfo) ValidateInfo() error { return validator.Error() } -// TeamAccessInfo implements CreatableInfo. -var _ CreatableInfo = &TeamAccessInfo{} +// Equals can be used to check if this *Info request (the desired state) matches the actual +// passed in as the argument. +func (r RepositoryInfo) Equals(actual InfoRequest) bool { + return reflect.DeepEqual(r, actual) +} + +// TeamAccessInfo implements InfoRequest and DefaultedInfoRequest (with a pointer receiver). +var _ InfoRequest = TeamAccessInfo{} +var _ DefaultedInfoRequest = &TeamAccessInfo{} // TeamAccessInfo contains high-level information about a team's access to a repository. type TeamAccessInfo struct { @@ -100,7 +110,7 @@ func (ta *TeamAccessInfo) Default() { } // ValidateInfo validates the object at {Object}.Set() and POST-time. -func (ta *TeamAccessInfo) ValidateInfo() error { +func (ta TeamAccessInfo) ValidateInfo() error { validator := validation.New("TeamAccess") // Make sure we've set the name of the team if len(ta.Name) == 0 { @@ -113,8 +123,15 @@ func (ta *TeamAccessInfo) ValidateInfo() error { return validator.Error() } -// DeployKeyInfo implements CreatableInfo. -var _ CreatableInfo = &DeployKeyInfo{} +// Equals can be used to check if this *Info request (the desired state) matches the actual +// passed in as the argument. +func (ta TeamAccessInfo) Equals(actual InfoRequest) bool { + return reflect.DeepEqual(ta, actual) +} + +// DeployKeyInfo implements InfoRequest and DefaultedInfoRequest (with a pointer receiver). +var _ InfoRequest = DeployKeyInfo{} +var _ DefaultedInfoRequest = &DeployKeyInfo{} // DeployKeyInfo contains high-level information about a deploy key. type DeployKeyInfo struct { @@ -140,7 +157,7 @@ func (dk *DeployKeyInfo) Default() { } // ValidateInfo validates the object at {Object}.Set() and POST-time. -func (dk *DeployKeyInfo) ValidateInfo() error { +func (dk DeployKeyInfo) ValidateInfo() error { validator := validation.New("DeployKey") // Make sure we've set the name of the deploy key if len(dk.Name) == 0 { @@ -154,3 +171,9 @@ func (dk *DeployKeyInfo) ValidateInfo() error { // the RepositoryClient. In the client, we make sure that they equal. return validator.Error() } + +// Equals can be used to check if this *Info request (the desired state) matches the actual +// passed in as the argument. +func (dk DeployKeyInfo) Equals(actual InfoRequest) bool { + return reflect.DeepEqual(dk, actual) +} From 8e6f973cd2449c123e93dc5b721169b8cddbeba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 16:42:38 +0300 Subject: [PATCH 17/19] Add badges :) --- README.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/README.md b/README.md index 9c51d26f..428055b7 100644 --- a/README.md +++ b/README.md @@ -2,5 +2,12 @@ [![godev](https://img.shields.io/static/v1?label=godev&message=reference&color=00add8)](https://pkg.go.dev/github.com/fluxcd/go-git-providers) [![build](https://github.com/fluxcd/go-git-providers/workflows/build/badge.svg)](https://github.com/fluxcd/go-git-providers/actions) +[![Go Report Card](https://goreportcard.com/badge/github.com/fluxcd/go-git-providers)](https://goreportcard.com/report/github.com/fluxcd/go-git-providers) +[![codecov.io](https://codecov.io/github/fluxcd/go-git-providers/coverage.svg?branch=master)] +(https://codecov.io/github/fluxcd/go-git-providers?branch=master) +[![LICENSE](https://img.shields.io/github/license/fluxcd/go-git-providers)](https://github.com/fluxcd/go-git-providers/blob/master/LICENSE) +[![Contributors](https://img.shields.io/github/contributors/fluxcd/go-git-providers)](https://github.com/fluxcd/go-git-providers/graphs/contributors) +[![Release](https://img.shields.io/github/v/release/fluxcd/go-git-providers?include_prereleases)](https://github.com/fluxcd/go-git-providers/releases/latest) +[![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg?style=flat-square)](https://github.com/fluxcd/go-git-providers/blob/master/CONTRIBUTING.md) The go-git-providers is a general-purpose Go client for interacting with Git providers APIs (e.g. GitHub, GitLab, Bitbucket). From d4b45c798c22af2e2d1f37ceb32bf988d0bd5a08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 16:58:29 +0300 Subject: [PATCH 18/19] fix badge --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 428055b7..d1d2b6e3 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,7 @@ [![godev](https://img.shields.io/static/v1?label=godev&message=reference&color=00add8)](https://pkg.go.dev/github.com/fluxcd/go-git-providers) [![build](https://github.com/fluxcd/go-git-providers/workflows/build/badge.svg)](https://github.com/fluxcd/go-git-providers/actions) [![Go Report Card](https://goreportcard.com/badge/github.com/fluxcd/go-git-providers)](https://goreportcard.com/report/github.com/fluxcd/go-git-providers) -[![codecov.io](https://codecov.io/github/fluxcd/go-git-providers/coverage.svg?branch=master)] -(https://codecov.io/github/fluxcd/go-git-providers?branch=master) +[![codecov.io](https://codecov.io/github/fluxcd/go-git-providers/coverage.svg?branch=master)](https://codecov.io/github/fluxcd/go-git-providers?branch=master) [![LICENSE](https://img.shields.io/github/license/fluxcd/go-git-providers)](https://github.com/fluxcd/go-git-providers/blob/master/LICENSE) [![Contributors](https://img.shields.io/github/contributors/fluxcd/go-git-providers)](https://github.com/fluxcd/go-git-providers/graphs/contributors) [![Release](https://img.shields.io/github/v/release/fluxcd/go-git-providers?include_prereleases)](https://github.com/fluxcd/go-git-providers/releases/latest) From 695828ddc139daed6fd7770a2099d02ff9f4bfa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lucas=20K=C3=A4ldstr=C3=B6m?= Date: Thu, 13 Aug 2020 18:20:49 +0300 Subject: [PATCH 19/19] Review feedback --- .golangci.yml | 2 +- bitbucket/doc.go | 2 +- github/resource_repository.go | 2 +- gitlab/doc.go | 2 +- gitprovider/gitprovider.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 4db6889e..59053504 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -46,7 +46,7 @@ linters: # - godox don't error although there are TODOs in the code - funlen - whitespace - # - wsl allow "non-ideomatic" whitespace formattings for now + # - wsl allow "non-idiomatic" whitespace formattings for now - goprintffuncname - gomnd - goerr113 diff --git a/bitbucket/doc.go b/bitbucket/doc.go index cb08392e..68545977 100644 --- a/bitbucket/doc.go +++ b/bitbucket/doc.go @@ -17,6 +17,6 @@ limitations under the License. package bitbucket import ( - // Dummy import until we have the implementation ready. + // TODO: Dummy import until we have the implementation ready. _ "github.com/ktrysmt/go-bitbucket" ) diff --git a/github/resource_repository.go b/github/resource_repository.go index e4e83a36..2f6787ae 100644 --- a/github/resource_repository.go +++ b/github/resource_repository.go @@ -136,7 +136,7 @@ func (r *userRepository) Reconcile(ctx context.Context) (bool, error) { return true, r.Update(ctx) } -// Delete deletes the current resource irreversible. +// Delete deletes the current resource irreversibly. // // ErrNotFound is returned if the resource doesn't exist anymore. func (r *userRepository) Delete(ctx context.Context) error { diff --git a/gitlab/doc.go b/gitlab/doc.go index 8d732db3..d7df82ab 100644 --- a/gitlab/doc.go +++ b/gitlab/doc.go @@ -17,6 +17,6 @@ limitations under the License. package gitlab import ( - // Dummy import until we have the implementation ready. + // TODO: Dummy import until we have the implementation ready. _ "github.com/xanzy/go-gitlab" ) diff --git a/gitprovider/gitprovider.go b/gitprovider/gitprovider.go index 54e315e6..84729773 100644 --- a/gitprovider/gitprovider.go +++ b/gitprovider/gitprovider.go @@ -66,7 +66,7 @@ type Updatable interface { // Deletable is an interface which all objects that can be deleted // using the Client implement. type Deletable interface { - // Delete deletes the current resource irreversible. + // Delete deletes the current resource irreversibly. // // ErrNotFound is returned if the resource doesn't exist anymore. Delete(ctx context.Context) error