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

feat(templates): the user can configure a url and ref to fetch templates from #566

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

vfried
Copy link
Contributor

@vfried vfried commented Aug 14, 2019

Addresses #561

I created the branch 'test-branch-manifests' inside our templates project for testing reasons.

In case there is no branch and url configured it fails. Specifications say that default should be master but if there is no URL it will fail anyway unless we want to set the url from our templates project as default along with master.

I had to modify also my public/config.json to make it work, i'm not sure something is missing in the configuration I made. My config file looks like this now (i put it here because is an untracked file):

{
"BASE_URL": "https://virginia.dev.renku.ch",
"GATEWAY_URL": "https://virginia.dev.renku.ch/api",
"WELCOME_PAGE": "IyMgV2VsY29tZSB0byBSZW5rdSB0aHJvdWdoIHRlbGVwcmVzZW5jZQpTb21lIGRlcGxveW1lbnQtc3BlY2lmaWMgaW5mb3JtYXRpb24gd2lsbCBiZSByZWFkIGZyb20gdGhlIHlvdXIgdmFsdWVzLnlhbWwgZmlsZSBhbmQgYmUgZGlzcGxheWVkIGFzIG1hcmtkb3duIGZpbGUuCg==",
"RENKU_VERSION": "latest",
"RENKU_TEMPLATES_URL": "https://github.com/repos/SwissDataScienceCenter/renku-project-template",
"RENKU_TEMPLATES_REF": "test-branch-manifests"
}

@vfried vfried requested a review from a team as a code owner August 14, 2019 09:58
@rokroskar
Copy link
Member

why use the api.github.com URL instead of just github.com?

@vfried
Copy link
Contributor Author

vfried commented Aug 14, 2019

why use the api.github.com URL instead of just github.com?

Because we are calling the github api, otherwise it fails. Unless we add the "api." string part ourselves.

From github api documentation:
image

@rokroskar
Copy link
Member

I think it would make more sense for whoever is deploying the chart to just put the URL of the repo - it's not that you usually put API URLs in configuration files.

@vfried
Copy link
Contributor Author

vfried commented Aug 14, 2019

I think it would make more sense for whoever is deploying the chart to just put the URL of the repo - it's not that you usually put API URLs in configuration files.

True, I will do the change so they only put the repo url.

@rokroskar rokroskar requested a review from a team August 14, 2019 11:15
@vfried vfried self-assigned this Aug 14, 2019
@vfried
Copy link
Contributor Author

vfried commented Aug 14, 2019

I think it would make more sense for whoever is deploying the chart to just put the URL of the repo - it's not that you usually put API URLs in configuration files.

Done, now the url should be https://github.com/repos/SwissDataScienceCenter/renku-project-template or similar

@rokroskar
Copy link
Member

should be just https://github.com/SwissDataScienceCenter/renku-project-template - without the repos in the middle

@vfried
Copy link
Contributor Author

vfried commented Aug 14, 2019

should be just https://github.com/SwissDataScienceCenter/renku-project-template - without the repos in the middle

True, I changed it so now it works without the "/repo" part. It will work now assuming that the users put the proper URL.

helm-chart/renku-ui/values.yaml Outdated Show resolved Hide resolved
rokroskar
rokroskar previously approved these changes Aug 14, 2019
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

It works for me.
I have just a suggestion: I noted you invoke fetchJson(${<url>}/git/trees/${<template>}) multiple times. It may be a good idea to create a single function and invoke it whenever needed.

return fetchJson(TemplatesReposUrl.TREES_MASTER)
client.getProjectTemplates = (renkuTemplatesUrl, renkuTemplatesRef) => {
const formatedApiURL = getApiURLfromRepoURL(renkuTemplatesUrl);
return fetchJson(`${formatedApiURL}/git/trees/${renkuTemplatesRef}`)
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi Aug 15, 2019

Choose a reason for hiding this comment

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

fetchJson(${<url>}/git/trees/${<template>}) --> function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. If i make a function to call the fetchJson function it will end up looking almost the same, and the exact same case only happens twice since the third case it's with the tree sha and it adds the ?recursive=1 at the end.

Copy link
Member

Choose a reason for hiding this comment

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

I thought it could make sense to have it only in one place so it's easier to understand that they are doing the same thing. The ?recursive=1 could easily be a parameter. Something like this

function getGitTrees(apiUrl, templateUrl, recursive=false) {
  let url = `${apiUrl}/git/trees/${templateUrl}`;
  if (recursive)
    url += "?recursive=1";
  return fetchJson(url);
}

Btw I am going to accept the PR as it is, feel free to use the function or not, it's not a big difference 🙂

// Promise which will resolve into the repository sub-tree
// which matches the desired version of the renku project template.
const subTreePromise = fetchJson(TemplatesReposUrl.TREES_MASTER)
const formatedApiURL = getApiURLfromRepoURL(renkuTemplatesUrl);
const subTreePromise = fetchJson(`${formatedApiURL}/git/trees/${renkuTemplatesRef}`)
Copy link
Member

Choose a reason for hiding this comment

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

fetchJson(${<url>}/git/trees/${<template>}) --> function

.then(data => data.tree.filter(obj => obj.path === projectTemplate)[0]['sha'])
.then(treeSha => fetchJson(`${TemplatesReposUrl.TREES}${treeSha}?recursive=1`));
.then(treeSha => fetchJson(`${formatedApiURL}/git/trees/${treeSha}?recursive=1`));
Copy link
Member

Choose a reason for hiding this comment

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

fetchJson(${<url>}/git/trees/${<template>}) --> function

docker-entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

if RENKU_VERSION is not used anywhere anymore, please remove it from the configuration files and charts

@ciyer ciyer requested a review from rokroskar August 16, 2019 07:36
@ciyer ciyer added this to the 0.6.3 milestone Aug 16, 2019
@ciyer ciyer merged commit f45d8a2 into master Aug 20, 2019
@ciyer ciyer deleted the 561-templates-ref branch August 29, 2019 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants