Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce caching for GitHub access method and add tests for the downloader #57

Merged
merged 3 commits into from
Aug 19, 2022
Merged

Introduce caching for GitHub access method and add tests for the downloader #57

merged 3 commits into from
Aug 19, 2022

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Aug 12, 2022 •
edited
Loading

This PR will wait until #40 is merged. Then we can rebase on it.

This PR does some refactoring around the usage of the Downloader and adds blob caching to the Github access method.

Manual Test

ocm --cred :type=github --cred :hostname=github.com  --cred token=${GITHUB_TOKEN} download resources ocm-github
github.com/skarlso/ocm-test/0.0.1/private-server: 272 byte(s) written
github.com/skarlso/ocm-test/0.0.1/server: 1594 byte(s) written

Without creds:

ocm download resources ocm-github-public-only
github.com/skarlso/ocm-test/0.0.1/server: 1594 byte(s) written

@gardener-robot
Copy link
Contributor

@Skarlso Thank you for your contribution.

@Skarlso Skarlso force-pushed the cache_github_access branch from 96e11f7 to 87387a6 Compare August 12, 2022 11:17
@gardener-robot gardener-robot added size/m Medium and removed size/xl labels Aug 12, 2022
@Skarlso Skarlso changed the title Cache GitHub access [WIP] Cache GitHub access Aug 12, 2022
@Skarlso Skarlso changed the title [WIP] Cache GitHub access [WIP] Introduce caching for GitHub access method and add tests for the downloader Aug 12, 2022
@gardener-robot gardener-robot added size/xl and removed size/m Medium labels Aug 12, 2022
@Skarlso Skarlso force-pushed the cache_github_access branch from 1975d2b to 1bd8122 Compare August 12, 2022 14:01
@gardener-robot gardener-robot added size/l Large and removed size/xl labels Aug 12, 2022
@Skarlso Skarlso force-pushed the cache_github_access branch 3 times, most recently from a73cb49 to 74f2934 Compare August 12, 2022 14:08
@gardener-robot gardener-robot added size/xl and removed size/l Large labels Aug 12, 2022
Skarlso
Skarlso commented Aug 12, 2022
View reviewed changes
pkg/contexts/ocm/accessmethods/github/method.go
&oauth2.Token{AccessToken: token},
)
httpclient = oauth2.NewClient(context.Background(), ts)
}
if u.Hostname() == "github.com" {
Copy link
Contributor Author

@Skarlso Skarlso Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will use an enterprise client with an incorrect URL (it adds api/v3 to the URL) in case of vanity URLs like k8s.io.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably can ignore this for now, but we need a better way of detecting URLs which will require an enterprise client.

@Skarlso Skarlso force-pushed the cache_github_access branch 5 times, most recently from b97a87c to f003be6 Compare August 12, 2022 14:25
@Skarlso Skarlso changed the title [WIP] Introduce caching for GitHub access method and add tests for the downloader Introduce caching for GitHub access method and add tests for the downloader Aug 12, 2022
@Skarlso Skarlso force-pushed the cache_github_access branch from f003be6 to 6428068 Compare August 12, 2022 14:25
@Skarlso Skarlso requested review from mandelsoft and yitsushi and removed request for mandelsoft August 12, 2022 14:25
@Skarlso Skarlso requested review from mandelsoft and removed request for yitsushi August 12, 2022 14:26
@Skarlso
Copy link
Contributor Author

Skarlso commented Aug 12, 2022

@yitsushi I can't assign multiple reviewers so pinging you here for exposure. :)

@gardener-robot
Copy link
Contributor

@mandelsoft You have pull request review open invite, please check

@Skarlso Skarlso requested a review from yitsushi August 16, 2022 07:04
@Skarlso Skarlso mentioned this pull request Aug 16, 2022
Merged
mandelsoft
mandelsoft previously requested changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@mandelsoft mandelsoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This downloader package is no access method, so the place does not seem to be a good choice.
May be we could just put it into the accessio package.

The provided default implementation, is it used somewhere?

pkg/contexts/ocm/accessmethods/github/method.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/accessmethods/github/method.go Outdated Show resolved Hide resolved
@Skarlso
Copy link
Contributor Author

Skarlso commented Aug 17, 2022

This downloader package is no access method, so the place does not seem to be a good choice.
May be we could just put it into the accessio package.

Sure, I'll move it.

The provided default implementation, is it used somewhere?

There is no default implementation. Which implementation do you have in mind?

@Skarlso Skarlso requested a review from a team as a code owner August 17, 2022 12:14
@Skarlso Skarlso requested review from achimweigel, reshnm and mandelsoft and removed request for a team August 17, 2022 12:14
@Skarlso Skarlso force-pushed the cache_github_access branch from edaf7af to ee4b955 Compare August 17, 2022 12:16
@Skarlso Skarlso force-pushed the cache_github_access branch from ee4b955 to 5ffdc0e Compare August 17, 2022 12:21
@Skarlso Skarlso dismissed mandelsoft’s stale review August 17, 2022 12:42

Please review new changes, thanks!

@Skarlso Skarlso merged commit e3a7502 into main Aug 19, 2022
@Skarlso Skarlso deleted the cache_github_access branch August 19, 2022 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandelsoft mandelsoft mandelsoft approved these changes

@yitsushi yitsushi yitsushi approved these changes

@achimweigel achimweigel Awaiting requested review from achimweigel achimweigel was automatically assigned from gardener/landscaper-maintainers

@reshnm reshnm Awaiting requested review from reshnm reshnm was automatically assigned from gardener/landscaper-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Skarlso @gardener-robot @yitsushi @mandelsoft