Skip to content

Commit 43632aa

Browse files
jakobmoellerdevjakobmoellerdev
authored
fix: make sure that ref is an option in the gitHub AccessMethod (#1406)
<!-- markdownlint-disable MD041 --> #### What this PR does / why we need it previously ref was lost in the access method. This adds it back in... #### Which issue(s) this PR fixes <!-- Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`. -->
1 parent 05ef296 commit 43632aa

20 files changed

Lines changed: 101 additions & 87 deletions

File tree

‎api/ocm/elements/artifactaccess/githubaccess/resource.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func Access[M any, P compdesc.ArtifactMetaPointer[M]](ctx ocm.Context, meta P, r
1919
meta.SetType(TYPE)
2020
}
2121

22-
spec := access.New(repo, eff.APIHostName, commit)
22+
spec := access.New(repo, eff.APIHostName, access.WithCommit(commit))
2323
// is global access, must work, otherwise there is an error in the lib.
2424
return genericaccess.MustAccess(ctx, meta, spec)
2525
}

‎api/ocm/extensions/accessmethods/github/README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ The type specific specification fields are:
3030

3131
- **`ref`** (optional) *string*
3232

33-
Original ref used to get the commit from
33+
Original ref used to get the commit from. mutually exclusive with `commit`.
3434

35-
- **`commit`** *string*
35+
- **`commit`** (optional) *string*
3636

37-
The sha/id of the git commit
37+
The sha/id of the git commit. mutually exclusive with `ref`.
3838

3939
### Go Bindings
4040

‎api/ocm/extensions/accessmethods/github/cli.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ func ConfigHandler() flagsets.ConfigOptionTypeSetHandler {
1111
options.RepositoryOption,
1212
options.HostnameOption,
1313
options.CommitOption,
14+
options.ReferenceOption,
1415
)
1516
}
1617

1718
func AddConfig(opts flagsets.ConfigOptions, config flagsets.Config) error {
1819
flagsets.AddFieldByOptionP(opts, options.RepositoryOption, config, "repoUrl")
1920
flagsets.AddFieldByOptionP(opts, options.CommitOption, config, "commit")
21+
flagsets.AddFieldByOptionP(opts, options.ReferenceOption, config, "ref")
2022
flagsets.AddFieldByOptionP(opts, options.HostnameOption, config, "apiHostname")
2123
return nil
2224
}
@@ -35,9 +37,9 @@ The type specific specification fields are:
3537
3638
- **<code>ref</code>** (optional) *string*
3739
38-
Original ref used to get the commit from
40+
Original ref used to get the commit from. Mutually exclusive with <code>ref</code>.
3941
4042
- **<code>commit</code>** *string*
4143
42-
The sha/id of the git commit
44+
The sha/id of the git commit. Mutually exclusive with <code>commit</code>.
4345
`

‎api/ocm/extensions/accessmethods/github/method.go‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ type AccessSpec struct {
6464
// Commit defines the hash of the commit
6565
Commit string `json:"commit"`
6666

67+
// Reference defines the original ref used to get the commit from
68+
// Mutually exclusive with Commit
69+
Reference string `json:"ref,omitempty"`
70+
6771
client *http.Client
6872
downloader downloader.Downloader
6973
}
@@ -87,13 +91,24 @@ func WithDownloader(downloader downloader.Downloader) AccessSpecOptions {
8791
}
8892
}
8993

94+
func WithCommit(commit string) AccessSpecOptions {
95+
return func(s *AccessSpec) {
96+
s.Commit = commit
97+
}
98+
}
99+
100+
func WithReference(ref string) AccessSpecOptions {
101+
return func(s *AccessSpec) {
102+
s.Reference = ref
103+
}
104+
}
105+
90106
// New creates a new GitHub registry access spec version v1.
91-
func New(repoURL, apiHostname, commit string, opts ...AccessSpecOptions) *AccessSpec {
107+
func New(repoURL, apiHostname string, opts ...AccessSpecOptions) *AccessSpec {
92108
s := &AccessSpec{
93109
ObjectVersionedType: runtime.NewVersionedTypedObject(Type),
94110
RepoURL: repoURL,
95111
APIHostname: apiHostname,
96-
Commit: commit,
97112
}
98113
for _, o := range opts {
99114
o(s)
@@ -156,8 +171,15 @@ type accessMethod struct {
156171
var _ accspeccpi.AccessMethodImpl = (*accessMethod)(nil)
157172

158173
func newMethod(c accspeccpi.ComponentVersionAccess, a *AccessSpec) (*accessMethod, error) {
159-
if err := validateCommit(a.Commit); err != nil {
160-
return nil, fmt.Errorf("failed to validate commit: %w", err)
174+
if a.Commit != "" && a.Reference != "" {
175+
return nil, fmt.Errorf("commit and ref cannot be specified at the same time")
176+
}
177+
178+
if a.Commit != "" {
179+
if err := validateCommit(a.Commit); err != nil {
180+
return nil, fmt.Errorf("failed to validate commit: %w", err)
181+
}
182+
a.Reference = a.Commit
161183
}
162184

163185
unparsed := a.RepoURL
@@ -295,7 +317,7 @@ func (m *accessMethod) setup() error {
295317

296318
func (m *accessMethod) getDownloadLink() (string, error) {
297319
link, resp, err := m.repositoryService.GetArchiveLink(context.Background(), m.owner, m.repo, github.Tarball, &github.RepositoryContentGetOptions{
298-
Ref: m.spec.Commit,
320+
Ref: m.spec.Reference,
299321
}, true)
300322
if err != nil {
301323
return "", err

‎api/ocm/extensions/accessmethods/github/method_test.go‎

Lines changed: 41 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,6 @@ var _ = Describe("Method", func() {
5959
expectedBlobContent []byte
6060
err error
6161
defaultLink string
62-
accessSpec *me.AccessSpec
6362
fs vfs.FileSystem
6463
expectedURL string
6564
clientFn func(url string) *http.Client
@@ -88,15 +87,6 @@ var _ = Describe("Method", func() {
8887
})
8988
}
9089

91-
accessSpec = me.New(
92-
"https://github.com/test/test",
93-
"",
94-
"7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f",
95-
me.WithClient(clientFn(expectedURL)),
96-
me.WithDownloader(&mockDownloader{
97-
expected: expectedBlobContent,
98-
}),
99-
)
10090
fs, err = osfs.NewTempFileSystem()
10191
Expect(err).To(Succeed())
10292
vfsattr.Set(ctx, fs)
@@ -107,28 +97,52 @@ var _ = Describe("Method", func() {
10797
vfs.Cleanup(fs)
10898
})
10999

100+
DescribeTable("AccessMethod",
101+
func(repoURL, apiHostname, commit, ref, expectedURL string) {
102+
accessSpec := me.New(
103+
repoURL,
104+
apiHostname,
105+
me.WithCommit(commit),
106+
me.WithReference(ref),
107+
me.WithClient(clientFn(expectedURL)),
108+
me.WithDownloader(&mockDownloader{
109+
expected: expectedBlobContent,
110+
}),
111+
)
112+
m, err := accessSpec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
113+
Expect(err).ToNot(HaveOccurred())
114+
content, err := m.Get()
115+
Expect(err).ToNot(HaveOccurred())
116+
Expect(content).To(Equal(expectedBlobContent))
117+
118+
},
119+
Entry("with commit", "https://github.com/test/test", "", "7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f", "", "https://api.github.com/repos/test/test/tarball/7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f"),
120+
Entry("with ref", "https://github.com/test/test", "", "", "refs/heads/main", "https://api.github.com/repos/test/test/tarball/refs/heads/main"),
121+
)
122+
110123
It("provides consumer id", func() {
124+
accessSpec := me.New(
125+
"https://github.com/test/test",
126+
"",
127+
me.WithCommit("7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f"),
128+
me.WithClient(clientFn(expectedURL)),
129+
me.WithDownloader(&mockDownloader{
130+
expected: expectedBlobContent,
131+
}),
132+
)
111133
m, err := accessSpec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
112134
Expect(err).ToNot(HaveOccurred())
113135
Expect(credentials.GetProvidedConsumerId(m)).To(Equal(credentials.NewConsumerIdentity(identity.CONSUMER_TYPE,
114136
identity.ID_HOSTNAME, "github.com",
115137
identity.ID_PATHPREFIX, "test/test")))
116138
})
117139

118-
It("downloads artifacts", func() {
119-
m, err := accessSpec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
120-
Expect(err).ToNot(HaveOccurred())
121-
content, err := m.Get()
122-
Expect(err).ToNot(HaveOccurred())
123-
Expect(content).To(Equal(expectedBlobContent))
124-
})
125-
126140
When("the commit sha is of an invalid length", func() {
127141
It("errors", func() {
128142
accessSpec := me.New(
129143
"hostname",
130144
"",
131-
"not-a-sha",
145+
me.WithCommit("not-a-sha"),
132146
me.WithClient(clientFn(expectedURL)),
133147
me.WithDownloader(&mockDownloader{
134148
expected: expectedBlobContent,
@@ -147,7 +161,7 @@ var _ = Describe("Method", func() {
147161
accessSpec := me.New(
148162
"hostname",
149163
"1234",
150-
"refs/heads/veryinteresting_branch_namess",
164+
me.WithCommit("refs/heads/veryinteresting_branch_namess"),
151165
me.WithClient(clientFn(expectedURL)),
152166
me.WithDownloader(&mockDownloader{
153167
expected: expectedBlobContent,
@@ -184,17 +198,17 @@ var _ = Describe("Method", func() {
184198
}
185199
})
186200
}
187-
accessSpec = me.New(
201+
})
202+
It("can use those to access private repos", func() {
203+
accessSpec := me.New(
188204
"https://github.com/test/test",
189205
"",
190-
"7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f",
206+
me.WithCommit("7b1445755ee2527f0bf80ef9eeb59a5d2e6e3e1f"),
191207
me.WithClient(clientFn(expectedURL)),
192208
me.WithDownloader(&mockDownloader{
193209
expected: expectedBlobContent,
194210
}),
195211
)
196-
})
197-
It("can use those to access private repos", func() {
198212
mcc := ocm.New(datacontext.MODE_INITIAL)
199213
src := &mockCredSource{
200214
Context: mcc.CredentialsContext(),
@@ -214,26 +228,10 @@ var _ = Describe("Method", func() {
214228
})
215229
})
216230

217-
When("GetCredentialsForConsumer returns an error", func() {
218-
It("errors", func() {
219-
mcc := ocm.New(datacontext.MODE_INITIAL)
220-
src := &mockCredSource{
221-
Context: mcc.CredentialsContext(),
222-
err: fmt.Errorf("danger will robinson"),
223-
}
224-
mcc.CredentialsContext().SetCredentialsForConsumer(credentials.NewConsumerIdentity(identity.CONSUMER_TYPE), src)
225-
_, err := accessSpec.AccessMethod(&mockComponentVersionAccess{
226-
ocmContext: mcc,
227-
})
228-
Expect(err).To(MatchError(ContainSubstring("danger will robinson")))
229-
Expect(src.called).To(BeTrue())
230-
})
231-
})
232-
233231
When("an enterprise repo URL is provided", func() {
234232
It("uses that domain and includes api/v3 in the request URL", func() {
235233
expectedURL = "https://github.tools.sap/api/v3/repos/test/test/tarball/25d9a3f0031c0b42e9ef7ab0117c35378040ef82"
236-
spec := me.New("https://github.tools.sap/test/test", "", "25d9a3f0031c0b42e9ef7ab0117c35378040ef82", me.WithClient(clientFn(expectedURL)))
234+
spec := me.New("https://github.tools.sap/test/test", "", me.WithCommit("25d9a3f0031c0b42e9ef7ab0117c35378040ef82"), me.WithClient(clientFn(expectedURL)))
237235
_, err := spec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
238236
Expect(err).ToNot(HaveOccurred())
239237
})
@@ -242,7 +240,7 @@ var _ = Describe("Method", func() {
242240
When("hostname is different from github.com", func() {
243241
It("will use an enterprise client", func() {
244242
expectedURL = "https://custom/api/v3/repos/test/test/tarball/25d9a3f0031c0b42e9ef7ab0117c35378040ef82"
245-
spec := me.New("https://github.tools.sap/test/test", "custom", "25d9a3f0031c0b42e9ef7ab0117c35378040ef82", me.WithClient(clientFn(expectedURL)))
243+
spec := me.New("https://github.tools.sap/test/test", "custom", me.WithCommit("25d9a3f0031c0b42e9ef7ab0117c35378040ef82"), me.WithClient(clientFn(expectedURL)))
246244
_, err := spec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
247245
Expect(err).ToNot(HaveOccurred())
248246
})
@@ -251,7 +249,7 @@ var _ = Describe("Method", func() {
251249
When("repoURL doesn't have an https prefix", func() {
252250
It("will add one", func() {
253251
expectedURL = "https://api.github.com/repos/test/test/tarball/25d9a3f0031c0b42e9ef7ab0117c35378040ef82"
254-
spec := me.New("github.com/test/test", "", "25d9a3f0031c0b42e9ef7ab0117c35378040ef82", me.WithClient(clientFn(expectedURL)))
252+
spec := me.New("github.com/test/test", "", me.WithCommit("25d9a3f0031c0b42e9ef7ab0117c35378040ef82"), me.WithClient(clientFn(expectedURL)))
255253
_, err := spec.AccessMethod(&cpi.DummyComponentVersionAccess{Context: ctx})
256254
Expect(err).ToNot(HaveOccurred())
257255
})

‎cmds/ocm/commands/ocmcmds/components/add/cmd_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
. "github.com/mandelsoft/goutils/testutils"
55
. "github.com/onsi/ginkgo/v2"
66
. "github.com/onsi/gomega"
7+
78
. "ocm.software/ocm/api/oci/testhelper"
89
. "ocm.software/ocm/cmds/ocm/testhelper"
910

‎docs/reference/ocm_add_resource-configuration.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -670,13 +670,13 @@ shown below.
670670

671671
- **<code>ref</code>** (optional) *string*
672672

673-
Original ref used to get the commit from
673+
Original ref used to get the commit from. Mutually exclusive with <code>ref</code>.
674674

675675
- **<code>commit</code>** *string*
676676

677-
The sha/id of the git commit
677+
The sha/id of the git commit. Mutually exclusive with <code>commit</code>.
678678

679-
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>
679+
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>, <code>--reference</code>
680680

681681
- Access type <code>helm</code>
682682

‎docs/reference/ocm_add_resources.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -682,13 +682,13 @@ shown below.
682682

683683
- **<code>ref</code>** (optional) *string*
684684

685-
Original ref used to get the commit from
685+
Original ref used to get the commit from. Mutually exclusive with <code>ref</code>.
686686

687687
- **<code>commit</code>** *string*
688688

689-
The sha/id of the git commit
689+
The sha/id of the git commit. Mutually exclusive with <code>commit</code>.
690690

691-
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>
691+
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>, <code>--reference</code>
692692

693693
- Access type <code>helm</code>
694694

‎docs/reference/ocm_add_source-configuration.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -670,13 +670,13 @@ shown below.
670670

671671
- **<code>ref</code>** (optional) *string*
672672

673-
Original ref used to get the commit from
673+
Original ref used to get the commit from. Mutually exclusive with <code>ref</code>.
674674

675675
- **<code>commit</code>** *string*
676676

677-
The sha/id of the git commit
677+
The sha/id of the git commit. Mutually exclusive with <code>commit</code>.
678678

679-
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>
679+
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>, <code>--reference</code>
680680

681681
- Access type <code>helm</code>
682682

‎docs/reference/ocm_add_sources.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -680,13 +680,13 @@ shown below.
680680

681681
- **<code>ref</code>** (optional) *string*
682682

683-
Original ref used to get the commit from
683+
Original ref used to get the commit from. Mutually exclusive with <code>ref</code>.
684684

685685
- **<code>commit</code>** *string*
686686

687-
The sha/id of the git commit
687+
The sha/id of the git commit. Mutually exclusive with <code>commit</code>.
688688

689-
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>
689+
Options used to configure fields: <code>--accessHostname</code>, <code>--accessRepository</code>, <code>--commit</code>, <code>--reference</code>
690690

691691
- Access type <code>helm</code>
692692

0 commit comments

Comments
 (0)