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

Add ability to clone private git repos #376

Merged
merged 3 commits into from
Feb 28, 2022
Merged

Add ability to clone private git repos #376

merged 3 commits into from
Feb 28, 2022

Conversation

vinamra28
Copy link
Member

@vinamra28 vinamra28 commented Feb 19, 2022 •
edited
Loading

Changes

Add ability to clone private git repos

With current codebase the ability to clone private repos was there but
it was incomplete -

  • Dockerfile for API was missing the openssh-clients
  • SSH URL needs to be parsed in case of private repos
    • Added one extra column ssh_url in catalogs table which stores the ssh url if passed
    • Add db-migration for the same

Add /readme and /yaml Endpoints

  • With providing support for reading private catalogs there is a need to
    serve the readme and yaml as the client of Hub API will not be able to
    access the Readme and Yaml from the git provider directly
  • Clients of Hub APIs will get the copy of Readme and YAML which was
    there at the time catalog refresh was done. This will ensure that the
    same copy is made available
  • The endpoints will return the string content after reading the file
    from the repo which was cloned in the target directory

Update UI to fetch README and YAML from Hub APIs

As of now Hub UI was fetching the readme and yaml directly from the git
provider such Github, Gitlab or Bitbucket. In case of private git repos,
UI was incapable of fetching the content directly as it requires
authentication. To solve this two endpoints were introduced by Hub API
/readme and /yaml which will do the needful. Updating UI to make API
calls to the Hub server instead of git provider directly.

Signed-off-by: vinamra28 vinjain@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Run API Unit Tests, Lint Checks, API Design, Golden Files with make api-check
  • Run UI Unit Tests, Lint Checks with make ui-check
  • Commit messages follow commit message best practices

See the contribution guide for more details.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2022
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2022
@vinamra28 vinamra28 force-pushed the ssh-repo-clone branch 3 times, most recently from 996509f to b47da88 Compare February 22, 2022 05:37
@vinamra28 vinamra28 marked this pull request as ready for review February 22, 2022 05:37
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2022
PuneetPunamiya
PuneetPunamiya reviewed Feb 22, 2022
View reviewed changes
api/v1/service/resource/resource.go Outdated
if err != nil {
return nil, err
}
readmePath := fmt.Sprintf("%s/%s/%s/%s/README.md", clonePath, strings.ToLower(p.Kind), p.Name, p.Version)
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this to
readmePath := fmt.Sprintf("%s/%s/%s/%s", catalogPath, strings.ToLower(p.Kind), p.Name, p.Version

readme := readmePath + "/README.md"

This would be more easy to read I think

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have any strong preferences. I am fine with either

PuneetPunamiya
PuneetPunamiya reviewed Feb 22, 2022
View reviewed changes
api/v1/service/resource/resource.go Outdated
if err != nil {
return nil, err
}
yamlPath := fmt.Sprintf("%s/%s/%s/%s/%s.yaml", clonePath, strings.ToLower(p.Kind), p.Name, p.Version, p.Name)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

PuneetPunamiya
PuneetPunamiya reviewed Feb 22, 2022
View reviewed changes
ui/src/api/index.ts Outdated Show resolved Hide resolved
pratap0007
pratap0007 reviewed Feb 22, 2022
View reviewed changes
api/pkg/app/data.go
@@ -38,6 +38,7 @@ type Catalog struct {
Org string
Type string
URL string
SshUrl string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SshUrl string
SSHURL string

Copy link
Member Author

Choose a reason for hiding this comment

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

@pratap0007 the reason why I have done it like SshUrl so that in config.yaml we can parse the value sshUrl

api/design/types/type.go Outdated Show resolved Hide resolved
api/pkg/db/migration/202202191725_add_ssh_url_column_in_catalogs_table.go Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
piyush-garg
piyush-garg reviewed Feb 22, 2022
View reviewed changes
api/pkg/git/git.go

cloneUrl := spec.URL

if spec.SSHUrl != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

so SSHURL will be given more priority if specified ?

Copy link
Member Author

Choose a reason for hiding this comment

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

as of now yes. Or should we try to attempt with both the urls?

sm43
sm43 reviewed Feb 23, 2022
View reviewed changes
api/pkg/git/git.go

cloneUrl := spec.URL

if spec.SSHUrl != "" {
Copy link
Member

Choose a reason for hiding this comment

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

how would user used their credentials for private repo?
this is just ssh url

Copy link
Member

Choose a reason for hiding this comment

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

may be mounting volume at certain path 🤔 i guess

Copy link
Member

Choose a reason for hiding this comment

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

? if yes?
wouldn't that work with http url?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sm43 yes user will have to volume mount their ssh keys.

wouldn't that work with http url?

no

@vinamra28
Add ability to clone private git repos
ed0e323 
With current codebase the ability to clone private repos was there but
it was incomplete -
- Dockerfile for API was missing the openssh-clients
- SSH URL needs to be parsed in case of private repos
  - Added one extra column ssh_url in catalogs table which stores the ssh url if passed
  - Add db-migration for the same

Signed-off-by: vinamra28 <vinjain@redhat.com>
@vinamra28 vinamra28 force-pushed the ssh-repo-clone branch 2 times, most recently from 5bac797 to c24b54a Compare February 24, 2022 06:51
@sm43
Copy link
Member

sm43 commented Feb 24, 2022 •
edited
Loading

@vinamra28
Could you document the changes required to done for cloning private repos?
volume mount and other changes in manifests

@vinamra28
Copy link
Member Author

@vinamra28 Could you document the changes required to done for cloning private repos? volume mount and other changes in manifests

I'll raise a follow up PR for that

@vinamra28
Add /readme and /yaml Endpoints
34934af 
- With providing support for reading private catalogs there is a need to
  serve the readme and yaml as the client of Hub API will not be able to
  access the Readme and Yaml from the git provider directly
- Clients of Hub APIs will get the copy of Readme and YAML which was
  there at the time catalog refresh was done. This will ensure that the
  same copy is made available
- The endpoints will return the string content after reading the file
  from the repo which was cloned in the target directory

Signed-off-by: vinamra28 <vinjain@redhat.com>
@vinamra28
Update UI to fetch README and YAML from Hub APIs
ad441bc 
As of now Hub UI was fetching the readme and yaml directly from the git
provider such Github, Gitlab or Bitbucket. In case of private git repos,
UI was incapable of fetching the content directly as it requires
authentication. To solve this two endpoints were introduced by Hub API
`/readme` and `/yaml` which will do the needful. Updating UI to make API
calls to the Hub server instead of git provider directly.

Signed-off-by: vinamra28 <vinjain@redhat.com>
@sm43 sm43 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2022
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@PuneetPunamiya
Copy link
Member

/lgtm
/meow

@tekton-robot
Copy link

@PuneetPunamiya: cat image

In response to this:

/lgtm
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2022
@pratap0007
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot merged commit e946915 into tektoncd:main Feb 28, 2022
@vinamra28 vinamra28 deleted the ssh-repo-clone branch June 23, 2022 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sm43 sm43 sm43 left review comments

@PuneetPunamiya PuneetPunamiya PuneetPunamiya left review comments

@pratap0007 pratap0007 pratap0007 left review comments

@piyush-garg piyush-garg piyush-garg left review comments

@sthaha sthaha Awaiting requested review from sthaha

@vdemeester vdemeester Awaiting requested review from vdemeester

Assignees

@pratap0007 pratap0007

@PuneetPunamiya PuneetPunamiya

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vinamra28 @sm43 @tekton-robot @PuneetPunamiya @pratap0007 @piyush-garg