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.

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

Places in code assume that repo is a Github repo #260

Open
jennydaman opened this issue Jan 14, 2022 · 5 comments
Open

Places in code assume that repo is a Github repo #260

jennydaman opened this issue Jan 14, 2022 · 5 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@jennydaman
Copy link
Collaborator

<a className="pf-m-link" href={`${pluginData.public_repo}/graphs/contributors`}>

We shouldn't make this assumption, might be hosted on better sites like Gitlab.

@jennydaman jennydaman added the help wanted Extra attention is needed label Jan 14, 2022
@zrthxn
Copy link
Member

zrthxn commented Jan 25, 2022

This assumption was made because in order to adapt this for different hosts (like gitlab, bitbucket, self-host etc) we'd need to have a different version of this component for each host's API, and that was needlessly complicated at the time.

This assumption is also made in the following places

const fetchRepoData = useCallback(async (repo) => {
const data = await fetch(`https://api.github.com/repos/${repo}`);
if (!data.ok) throw new GithubAPIRepoError(repo, data);
// Has to be done to avoid reassigning `data`
// eslint-disable-next-line no-return-await
return (await data.json());
}, []);

const fetchReadme = useCallback(async (repo) => {
const ghreadme = await fetch(`https://api.github.com/repos/${repo}/readme`);
if (!ghreadme.ok) throw new GithubAPIReadmeError(repo, ghreadme);
// Has to be done since key is snake_case in data coming from Github
// eslint-disable-next-line camelcase
const { download_url, content } = await ghreadme.json();
const file = Buffer.from(content, "base64").toString("utf8");
const type = download_url.split('.').reverse().shift();
return { file, type };
}, []);

@jennydaman
Copy link
Collaborator Author

Thanks @zrthxn!

This issue is mostly for tracking purposes, it's good for now but somewhere down the line we'd like for the ChRIS store to support the functionality here (of storing and serving READMEs).

As you mentioned, self-hosted git repos will not be easy to support, and this can be a common use case (hospitals need to host private or proprietary plugins internally).

@sherrif10
Copy link

@jennydaman @zrthxn Am intrested in working on this task , is it ready for work

@jennydaman
Copy link
Collaborator Author

@sherrif10 thanks for showing interest in this issue!

The ideal solution (to have the README come from the ChRIS Store backend) would be hard to implement, but we can improve on the current implementation. We can try:

  • get README from Github if (and only if) its public_repo is a github.com link
  • show a pretty error screen if README cannot be obtained

@sherrif10
Copy link

Thanks @jennydaman will share a pull request for more guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants