-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Allow using the --from flag to get a template from a github release #7453
Conversation
b90b1fa
to
4a7aae4
Compare
@@ -181,35 +182,77 @@ func (t *templateClient) getGitHubFileContent(rURL *url.URL) ([]byte, error) { | |||
|
|||
// Extract all the info from url split. | |||
owner := urlSplit[0] | |||
repository := urlSplit[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, what is the reason for renaming the variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return getGithubFileContentFromCode(ghClient, rURL.Path, owner, repo, path, branch) | ||
|
||
case "releases": // get a github release asset | ||
tag := urlSplit[4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to extract it above with all other variables related to the URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I tend not to do that, because the meaning of the 4th and 5th path components is different between these two cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with not extracting them early. Those values hold different meaning under blob
and releases
cases.
func getGithubAssetFromRelease(ghClient *github.Client, path string, owner string, repo string, tag string, assetName string) ([]byte, error) { | ||
release, _, err := ghClient.Repositories.GetReleaseByTag(ctx, owner, repo, tag) | ||
if err != nil { | ||
return nil, handleGithubErr(err, "failed to get release '%s' from %s%s repository", tag, owner, repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil, handleGithubErr(err, "failed to get release '%s' from %s%s repository", tag, owner, repo) | |
return nil, handleGithubErr(err, "failed to get release '%s' from %s/%s repository", tag, owner, repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
return content, nil | ||
} | ||
|
||
func getGithubAssetFromRelease(ghClient *github.Client, path string, owner string, repo string, tag string, assetName string) ([]byte, error) { | ||
release, _, err := ghClient.Repositories.GetReleaseByTag(ctx, owner, repo, tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check for a non-nil release
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
4a7aae4
to
fd24c24
Compare
@alexander-demicev - could you please review again, after the fixes? |
return getGithubFileContentFromCode(ghClient, rURL.Path, owner, repo, path, branch) | ||
|
||
case "releases": // get a github release asset | ||
tag := urlSplit[4] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with not extracting them early. Those values hold different meaning under blob
and releases
cases.
var rc io.ReadCloser | ||
for _, asset := range release.Assets { | ||
if asset.GetName() == assetName { | ||
rc, _, err = ghClient.Repositories.DownloadReleaseAsset(ctx, owner, repo, asset.GetID(), ghClient.Client()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you will also have to handle the redirects when working with release assets. Similar to how we handle the redirects here:
https://github.com/nunnatsa/cluster-api/blob/fd24c247c3ae9b32fed236f544088486ec6331cf/cmd/clusterctl/client/repository/repository_github.go#L331-L344
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you will also have to handle the redirects when working with release assets. Similar to how we handle the redirects here: https://github.com/nunnatsa/cluster-api/blob/fd24c247c3ae9b32fed236f544088486ec6331cf/cmd/clusterctl/client/repository/repository_github.go#L331-L344
Thanks @ykakarap. I'm not sure it's needed. According to the DownloadReleaseAsset
documentation:
followRedirectsClient can be passed to download the asset from a redirected location. Passing http.DefaultClient is recommended unless special circumstances exist, but it's possible to pass any http.Client. If nil is passed the redirectURL will be returned instead.
I added unit test to check the redirect scenario. Also, This code is working with the real github, that actually responses with redirect. If you think it's a must, I'll add it, but it's not needed, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem to work for redirected assets as well. @fabriziopandini Do you know why we are redirecting here: https://github.com/nunnatsa/cluster-api/blob/fd24c247c3ae9b32fed236f544088486ec6331cf/cmd/clusterctl/client/repository/repository_github.go#L331-L344?
Do you know of a case that is not covered by the standard DownloadReleaseAsset
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ykakarap we implemented this because this is one of the use case allowed by the return parameters of the GitHub API, but AFAIK we did not investigate why this parameter exists / in which case they support.
Unless we collect evidence this is unnecessary, I strongly suggest to handle redirect also here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fabriziopandini. According to the DownloadReleaseAsset
documentation I copied above, the redirect is only returned if the followRedirectsClient parameter is empty. This is, I guess, if you want to implement the redirect logic. But if you pass the client, the function will do it for you. This is how I understand it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk, I double checked the DownloadReleaseAsset implementation and it seems that this is not necessary if we are passing a followRedirectsClient.
In a separated PR we can also eventually clean up also the bit of code @ykakarap was referencing to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a follow-up issue to track the clean up work. #7942
fd24c24
to
245acc9
Compare
Thanks @sbueringer - I didn't know #7371, but I just read it. As far I can see, not only this PR is not conflicts with 7371, but it is even more important with it, because with 7371, blob github link will work, non-github link will work, but github release asset links will fail - and so #7455 is even more meaningful bug. |
They are not in conflict with each other. They are complimentary. I agree with @nunnatsa that having both 7371 and this PR would be beneficial as they would cover a broader set of cases. |
Sounds good! Thx for checking |
/cc @Jont828 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
This will be a nice addition :)
@@ -181,35 +182,81 @@ func (t *templateClient) getGitHubFileContent(rURL *url.URL) ([]byte, error) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot select the correct line here but the error in l.179 should be adjusted as now we support a blob as well as a release asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ykakarap - fixed.
var rc io.ReadCloser | ||
for _, asset := range release.Assets { | ||
if asset.GetName() == assetName { | ||
rc, _, err = ghClient.Repositories.DownloadReleaseAsset(ctx, owner, repo, asset.GetID(), ghClient.Client()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem to work for redirected assets as well. @fabriziopandini Do you know why we are redirecting here: https://github.com/nunnatsa/cluster-api/blob/fd24c247c3ae9b32fed236f544088486ec6331cf/cmd/clusterctl/client/repository/repository_github.go#L331-L344?
Do you know of a case that is not covered by the standard DownloadReleaseAsset
function?
245acc9
to
3c66590
Compare
Hi @fabriziopandini. What do you think about the redirect question from @ykakarap, here? #7453 (comment) |
Sorry for the delay, answered in the thread above |
Currently, `clusterctl generate cluster`'s `--from` flag only support yaml files from github code. Trying to get template from a rlease tag is failing. This PR adds the option to use the `--form` also for release assets. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
rebased (fix conflicts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, overall this make sense to me. I'll try to test this out locally when I get the chance, were you able to get this working by running a copy of clusterctl built off of this code?
yes |
LGTM label has been added. Git tree hash: 018505cf99c94f0d5cc398e6b24e2b18ab452b3b
|
Did a final pass and this is good to go. /lgtm Great work! Thank you for adding the release asset support. 🚀 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
Currently,
clusterctl generate cluster
's--from
flag only supports yaml files from github code (blob). Trying to get a template asset from a release tag is failing.This PR adds the option to use the
--form
also for release assets.Fixes #7455
Signed-off-by: Nahshon Unna-Tsameret nunnatsa@redhat.com