Github Access method #36
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Skarlso Thank you for your contribution. |
Skarlso
force-pushed
the
github_access_method
branch
from
bd99448 to
d6c5c68
Compare
Skarlso
force-pushed
the
github_access_method
branch
from
d6c5c68 to
b6bba92
Compare
|
Output for make test: |
Skarlso
force-pushed
the
github_access_method
branch
from
06c933a to
c0863c2
Compare
mandelsoft
requested changes
Aug 2, 2022
| ) | ||
|
|
||
| // Type is the access type of GitHub registry. | ||
| const Type = "github" |
There was a problem hiding this comment.
That contradicts with other types like ociRegistry, not? If we call this GitHub should we call the others like OCIRegistry?
There was a problem hiding this comment.
Potentially,
so far the Consumer type for OCI is also complete Camel Case, while the access method types always start with a lower case letter.
We have to decide, whether we want to stick to this or align it.
Another related problem:
The convention for labelling Matcher types is as follows:
- by default a matcher with the label of the comsumer type is used
- there are non-consumer type matchers that use lower case names to not conflict with consumer types.
So, if we decide to use lower case consumer types. we have to look for another way to distinguish them form the non consumer type matchers.
mandelsoft
reviewed
Aug 3, 2022
|
|
||
| When("the commit sha is of the right length but contains invalid characters", func() { | ||
| It("errors", func() { | ||
| accessMethod := ocmgithub.New( |
| Expect(err).To(MatchError(ContainSubstring("commit contains invalid characters for a SHA"))) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
just to be complete, you could add some test passing credentials:
- create a local credentials context
- compose an oci context using this
- compose an ocm context using these
- set credentials for a consumer
- and check whether the credentials reach the downloader
There was a problem hiding this comment.
👍 Agreed, I'm actually working on that right now. :)
There was a problem hiding this comment.
Sort of done.
To outline the problem would take too long, but basically, the http client is buried in the github library and cannot be nicely mocked out unless I do something in the production code, which is not okay.
Even if I try setting up some RoundTrip mocks, the oauth2 will override that and ignore the base if another token provider exist.
Therefor, we only verify that the cred is being called at least and check what happens when it errors.
Skarlso
force-pushed
the
github_access_method
branch
4 times, most recently
from
49ecfdd to
265143d
Compare
Skarlso
force-pushed
the
github_access_method
branch
from
265143d to
e538d78
Compare
mandelsoft
approved these changes
Aug 8, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #20
Special notes for your reviewer:
The following things have been changed:
githubThe following manual test has been applied:
resources.yaml
Then try downloading resources with this patch using the following component descriptor:
Using the following command:
Update the github paths accordingly and set
GITHUB_TOKENenvironment property.Release note: